-
Notifications
You must be signed in to change notification settings - Fork 61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cleanup of Clang-Tidy warnings in Applewin #179
base: master
Are you sure you want to change the base?
Conversation
for the bool section, that would be moving the wrong way - at least as i'm parsing how you've written it out. You don't want integer literals in place of logical boolean values. These are the clang-tidy options that impact boolean checking:
make sure that your clang-tidy is enabling them (i think you can get away with just "modernize..." for most of this, though "bugprone..." might be good as well) |
It is better to use Running with the options you suggested, it still recommends the opposite. $ clang-tidy src/Applewin.cpp -checks=bugprone-bool-pointer-implicit-conversion,readability-implicit-bool-conversion,readability-implicit-bool-cast,readability-simplify-boolean-expr,modernize-use-bool-literals
...
/home/maxolasersquad/linapple/src/Applewin.cpp:109:16: warning: converting integer literal to bool, use bool literal instead [modernize-use-bool-literals]
bool restart = 0;
... |
No, i think we're in agreement- I just noted that the way you wrote it initially was confusing to me without an explicit "should be" (should be using true/false instead of...) but for clarity, yes, it should be boolean values and not integer literals. |
Ah, I see. Thanks. |
ea9df16
to
c04540d
Compare
I updated my original comment for clarity. |
I added a |
Since it's not too impacting to just add the option in the Makefile, like...
why not add it as a separate target with no dependency linkage, so that it can be worked manually by folk who are interested, but doesn't impact the build process? (currently generates 2842 warnings!) |
after playing around with clang-tidy, and doing some cleanups, I've got things down to:
I did end up removing "readability-implicit-bool-conversion" from .clang-tidy, as it conflicts with modernize-use-bool-literals https://github.com/akhepcat/linapple has my current approach. there's... a lot of fixes in place. |
c04540d
to
c2b8614
Compare
c2b8614
to
7998ca9
Compare
@akhepcat, I added the new target in Makefile and also removed |
This resolves some warning provided by Clang-Tidy. I only targeted Applwin.(c/h) in this commit. I have tested the code compiles and could play Cave of Time.
Primarily changes are
bool
s now usetrue
andfalse
instead of1
and0
.time
in place of the ctime.h
.NULL
withnullptr
.There are other changes than this. This should definitely be inspected by someone with more cpp experience than me who can evaluate and test the likely impacts of this in more detail. Until then I'm leaving this as a draft for review. If this looks good I'll take the time to update the other files.