Add more compiler warnings #456
Labels
No labels
Area/build-packaging
Area/cli
Area/evaluator
Area/fetching
Area/flakes
Area/language
Area/profiles
Area/protocol
Area/releng
Area/remote-builds
Area/repl
Area/store
bug
crash 💥
Cross Compilation
devx
docs
Downstream Dependents
E/easy
E/hard
E/help wanted
E/reproducible
E/requires rearchitecture
imported
Needs Langver
OS/Linux
OS/macOS
performance
regression
release-blocker
RFD
stability
Status
blocked
Status
invalid
Status
postponed
Status
wontfix
testing
testing/flakey
ux
No milestone
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: lix-project/lix#456
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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.
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
andNixOS
. 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
andclang
and how to add them? From there on, I can probably just start fixing the code to get rid of these warnings.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.
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 :)
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.
Ok. So when I added
-Wall
and-Wextra
I found 3 warnings (ordered by number of warnings):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?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.
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
.This issue was mentioned on Gerrit on the following CLs:
I have created 2 changes:
Both of them get a "❌ Verified -1" and then under "Checks" I see:
I have no idea what this means or what I can do to fix it.
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.
Ok, thanks for clarifying. I'll wait for it to be resolved.