ci: add clang-tidy and clang-tidy-fix targets#242
ci: add clang-tidy and clang-tidy-fix targets#242andreiltd merged 28 commits intobytecodealliance:mainfrom
Conversation
|
Some quick responses:
Yes, I absolutely think we do
I haven't looked at them, but as long as none stick out as very annoying, I'm all for sticking with formatting defaults.
Yes :)
Yes :) The reason for the last two "yes"s is that that'll help avoid the situation where patches mix functional and formatting changes, making it much harder to identify the former. |
|
Thanks! OK, I will move this PR to draft then and try to come up with sensible lint defaults and fixes. |
|
This looks good to me, fwiw. Feel free to un-draft and land this once you feel comfortable with it |
This is needed to build compile_command.json file that clang-tidy uses.
These are various fixes reported by clang-analyzer regarding memory leaks, unsafe functions and inconsistent use use of new and free.
This changes script loader paths to use owned string instead of raw pointers to fix potential memory leaks reported by clang. The tradeof is that now we call c_str in few places, but the impact on performance should be negligible since we call this only once.
This patch enables cppcoreguidelines but disables all the lints that don't offer auto fixes.
This patch enables modernize lints and disable all lints from this group that are not offering auto fix.
This fixes linter error, where clang complains about using new/free instead of new/delete.
|
Hey @tschneidereit, while this PR is rather large, I've tried my best to keep individual commits logically separated. Each commit includes:
I've also added a CI step to run the |
tschneidereit
left a comment
tschneidereit
left a comment
There was a problem hiding this comment.
Thank you for this! I skimmed most of the files, and looked more closely at the first few (up to blob.cpp).
Mostly, the changes look very good to me. There are a few weird formatting issues though, where however you inserted or removed curly braces seems to have removed line breaks. It'd be great if you could look for those with a regex and fix them in one go.
Additionally, I left a comment on the first occurrence of a bool check that has been expanded to a comparison, in that case with nullptr. I'm not convinced we want those changes, myself. Ironically enough, in many cases I don't even think they're somehow "more correct": I don't think comparing a RootedObject or HandleObject with nullptr is particularly closer to some ground truth than comparing it to true (and then shortening the latter).
As I said in the inline comment, I'm not necessarily asking you to revert all those changes—but I also wouldn't be opposed to it 😬 I guess for now I'm more interested in hearing the reasoning, because I might simply be wrong.
Maybe it's worth, to just run |
Co-authored-by: Till Schneidereit <till@tillschneidereit.net>
|
I kept my janitor hat on for a little bit longer and cleaned up While disabling this warning is not ideal, starting from version 20.0, clang introduces the suppression maps option so we can selectively disable the warning only in problematic header files: https://clang.llvm.org/docs/WarningSuppressionMappings.html If it's OK, I will add an issue to remove the |
andreiltd
changed the title
|
Hey @tschneidereit, let me know if you wanted to have another pass on this or else maybe we can get this landed? |
|
I wasn't sure whether you were done iterating on it. Will take a look now. |
|
I will make a follow up PR where we parallelize the checks. This change is a bit controversial: #242 (comment) so I wanted to double check if you're on board to proceed 🙂 |
tschneidereit
left a comment
tschneidereit
left a comment
There was a problem hiding this comment.
I got through the files up to and including headers.cpp. That one contained some changes in behavior that make me nervous in this kind of PR.
Can you point out commits that might contain these kinds of changes, instead of being pure formatting changes or otherwise trivial? Otherwise this will take a long time to fully get through, I'm afraid :/
Ah, I had been wondering why it was okay to disable these checks. That all makes sense to me, and as long as you/we don't forget to apply that change once we can update SDKs, I'm okay with this plan. Getting rid of all the pragmas sure is nice! |
Co-authored-by: Till Schneidereit <till@tillschneidereit.net>
2 participants
This enables clang based linting over c++ codebase. There are two new targets:
just lint- runs clang-tidy on the codebasejust lint-fix- runs clang-tidy with-fixoption to automatically fix issuesThe
.clang-tidyfile contains minimal configuration to get us started. See enabledChecks.I was hoping to start the discussion on linting StarlingMonkey in general:
I hope we can review the errors from the linter and decide which are just the noise and which we want to fix.
cc: @tschneidereit