Add more compiler warnings

jade commented

2024-07-23 20:55:55 +00:00

Owner

This was unrelated content in #38, which I'm splitting out for visibility here.

Action plan:
Go through the list of lints in gcc and clang and add them one by one. Once you have about 20 fixes of lints, submit a CL with that flag addition. Then add more flags.

We are eating an elephant bite by bite on this.

This was unrelated content in https://git.lix.systems/lix-project/lix/issues/38, which I'm splitting out for visibility here. Action plan: Go through the list of lints in gcc and clang and add them one by one. Once you have about 20 fixes of lints, submit a CL with that flag addition. Then add more flags. We are eating an elephant bite by bite on this.

jade added the

stability

E/easy

E/help wanted

labels 2024-07-23 20:55:55 +00:00

lulu-berlin commented

2024-10-06 16:26:28 +00:00

Hello, @jade.

I would like to contribute and thought I'd pick an easy task to get started and thought I could start here.

I know rust quite well and basic C++ (I used it a long time ago, so I'm not very familiar with all of the new features). I also use nix and NixOS. I scrolled through the wiki and the code base and I think I got a general picture of how things work in this project. I am able to build and run the tests.

Regarding this issue, could you please show me the lists of lints in gcc and clang and how to add them? From there on, I can probably just start fixing the code to get rid of these warnings.

Hello, @jade. I would like to contribute and thought I'd pick an easy task to get started and thought I could start here. I know rust quite well and basic C++ (I used it a long time ago, so I'm not very familiar with all of the new features). I also use `nix` and `NixOS`. I scrolled through the wiki and the code base and I think I got a general picture of how things work in this project. I am able to build and run the tests. Regarding this issue, could you please show me the lists of lints in `gcc` and `clang` and how to add them? From there on, I can probably just start fixing the code to get rid of these warnings.

jade commented

2024-10-06 17:15:27 +00:00

Author

Owner

at the current point in time, gcc is unsupported due to years old unfixed compiler bugs and we are clang only until the gcc coroutines work properly.

here's the page with lints: https://clang.llvm.org/docs/DiagnosticsReference.html. unfortunately it looks like they're not split out by language (stuff like -Warc-* is objective c only and doesn't affect us), so i think the most reasonable course of action is to temporarily add -Wall -Wextra to the compiler flags in meson.build, then pick a specific warning or two you think you can fix and fix all of that warning.

once you've picked a warning you want to add to the permanent list, check if it's in both gcc and clang under the same flag (Google helps, and so can just compiling an empty .cpp file with g++) and if so add it to the list of always on warnings in the main meson.build.

at the current point in time, gcc is unsupported due to years old unfixed compiler bugs and we are clang only until the gcc coroutines work properly. here's the page with lints: https://clang.llvm.org/docs/DiagnosticsReference.html. unfortunately it looks like they're not split out by language (stuff like `-Warc-*` is objective c only and doesn't affect us), so i think the most reasonable course of action is to temporarily add -Wall -Wextra to the compiler flags in meson.build, then pick a specific warning or two you think you can fix and fix all of that warning. once you've picked a warning you want to add to the permanent list, check if it's in both gcc and clang under the same flag (Google helps, and so can just compiling an empty .cpp file with g++) and if so add it to the list of always on warnings in the main meson.build.

👍 1

lulu-berlin commented

2024-10-06 17:47:21 +00:00

Thanks for the quick reply.

So I guess the specification in the issue should be amended to refer only to clang.

I'm assuming the flags -Wall and -Wextra should be added in meson.build under add_project_arguments().

I did that and got lots of warnings when building, so I'll get to it :)

Thanks for the quick reply. So I guess the specification in the issue should be amended to refer only to `clang`. I'm assuming the flags `-Wall` and `-Wextra` should be added in [meson.build under add_project_arguments()](https://git.lix.systems/lix-project/lix/src/branch/main/meson.build#L484). I did that and got lots of warnings when building, so I'll get to it :)

jade commented

2024-10-06 19:02:10 +00:00

Author

Owner

we don't necessarily want to break gcc on purpose and we don't want to inadvertently use flags that don't exist on gcc, nor fail to add ones we can add; they may fix their compiler, so it's good to check if flags exist in both and add them to the flags for all compilers if so.

but there's no need to test lix itself with gcc as we know it has some combination of internal compiler error and miscompilation on gcc both related to coroutine bugs.

we don't necessarily want to break gcc on purpose and we don't want to inadvertently use flags that don't exist on gcc, nor fail to add ones we can add; they may fix their compiler, so it's good to check if flags exist in both and add them to the flags for all compilers if so. but there's no need to test lix itself with gcc as we know it has some combination of internal compiler error and miscompilation on gcc both related to coroutine bugs.

lulu-berlin commented

2024-10-06 20:18:23 +00:00

Ok. So when I added -Wall and -Wextra I found 3 warnings (ordered by number of warnings):

-Wunused-parameter
-Wmissing-field-initializers
-Wsign-compare

I have fixed the last 2 and committed them into a branch in my fork: https://git.lix.systems/lulu-berlin/lix/src/branch/456/add-more-compiler-warnings

I don't seem to have the permissions to create a pull request. Am I doing anything wrong?

In the branch that I mentioned, there are 2 commits for each warning:

Regarding -Wunused-parameter, I'm not sure how to go about this. Am I supposed to remove the unused parameters from the function signature and adapt all call sites?

Ok. So when I added `-Wall` and `-Wextra` I found 3 warnings (ordered by number of warnings): - -Wunused-parameter - -Wmissing-field-initializers - -Wsign-compare I have fixed the last 2 and committed them into a branch in my fork: https://git.lix.systems/lulu-berlin/lix/src/branch/456/add-more-compiler-warnings I don't seem to have the permissions to create a pull request. Am I doing anything wrong? In the branch that I mentioned, there are 2 commits for each warning: - [-Wsign-compare](https://git.lix.systems/lulu-berlin/lix/commit/1224476defc5efb1ba0ff8ad2e86b83c6018b8d9) - [-Wmissing-field-initializers](https://git.lix.systems/lulu-berlin/lix/commit/6756a067c254cd80a707369f5b7096f26da14e9b) Regarding `-Wunused-parameter`, I'm not sure how to go about this. Am I supposed to remove the unused parameters from the function signature and adapt all call sites?

jade commented

2024-10-06 21:32:11 +00:00

Author

Owner

We don't use forgejo for code review on lix itself at all. There's a guide in the getting started message from the dev shell (it's on the wiki) on how to submit code to Gerrit for review.

I would probably consider not bothering with the unused parameter warning; if that's all that's stopping us adding -Wall -Wextra, let's just disable it. My experience is that that warning is usually counterproductive, but the fix is usually deleting the parameter name and leaving the type in place.

We don't use forgejo for code review on lix itself at all. There's a guide in the getting started message from the dev shell (it's on the wiki) on how to submit code to Gerrit for review. I would probably consider not bothering with the unused parameter warning; if that's all that's stopping us adding -Wall -Wextra, let's just disable it. My experience is that that warning is usually counterproductive, but the fix is usually deleting the parameter name and leaving the type in place.

👍 1

lulu-berlin commented

2024-10-06 21:39:26 +00:00

I'm reading the wiki about Gerrit and trying to figure it out.

Ok, so I'll add -Wall and -Wextra and then -Wno-unused-parameter.

I'm reading the wiki about Gerrit and trying to figure it out. Ok, so I'll add `-Wall` and `-Wextra` and then `-Wno-unused-parameter`.

lix-bot commented

2024-10-06 22:40:00 +00:00

Member

This issue was mentioned on Gerrit on the following CLs:

commit message in cl/2044 ("Fix gcc warning -Wmissing-field-initializers")

This issue was mentioned on Gerrit on the following CLs: * commit message in [cl/2044](https://gerrit.lix.systems/c/lix/+/2044) ("Fix gcc warning -Wmissing-field-initializers")

lulu-berlin commented

2024-10-06 22:44:20 +00:00

I have created 2 changes:

Both of them get a "❌ Verified -1" and then under "Checks" I see:

Error while fetching results for checks:
Error message from plugin 'checks': {}

I have no idea what this means or what I can do to fix it.

I have created 2 changes: - https://gerrit.lix.systems/c/lix/+/2043 - https://gerrit.lix.systems/c/lix/+/2044 Both of them get a "❌ Verified -1" and then under "Checks" I see: > Error while fetching results for checks: > Error message from plugin 'checks': {} I have no idea what this means or what I can do to fix it.

jade commented

2024-10-07 04:38:58 +00:00

Author

Owner

that lack of appropriate message with the Verified-1 vote, is our ci infrastructure being temporarily busted due to ongoing refactoring. @raito is the one doing that and may or may not be aware why it happened.

*that* lack of appropriate message with the Verified-1 vote, is our ci infrastructure being temporarily busted due to ongoing refactoring. @raito is the one doing that and may or may not be aware why it happened.

👍 1

lulu-berlin commented

2024-10-07 04:43:18 +00:00

Ok, thanks for clarifying. I'll wait for it to be resolved.

Add more compiler warnings #456