- User Since
- Sep 22 2021, 11:35 PM (105 w, 4 d)
Wed, Sep 27
I still think even if we can subset this, for whatever we're going to turn into a hard error, it should be a warning-as-error in system headers first for at least a release. (so perhaps the transition should look like: null (no diagnostic) -> warning -> warning-default-to-error -> warning-default-to-error-even-in-system-headers -> hard error)
Sat, Sep 23
Hello! Just wanted to check if there's any blockers for merging this patch? We are now on Clang 18, i.e. 2 releases after the warning was introduced, so IMO I believe it's a good time to turn it into a hard error and test it in the wild.
Thu, Sep 21
Tue, Sep 5
Thanks for the patch! I must admit I don't fully understand what problem it solves, i.e. parameters are already optional today (one just simply doesn't specify them in the config file). Why would we want to explicitly spell out parameters with some default value that doesn't correspond to what actually gets applied in the check?
Sun, Sep 3
LGTM, thanks for fixing!
Aug 31 2023
Aug 14 2023
Aug 6 2023
Do you have plans to also detect the bugprone scenario described in the Notes here?
Aug 5 2023
LGTM, minor question that can be fixed post-review unless you want to discuss further!
Strange, it seems the alias was never fully added?
Jul 30 2023
What about the use case of privately inheriting std::array, and overriding the operator there with bounds check? I believe there shouldn't be warnings there.
Remove extra newline
Jul 27 2023
LGTM, thanks for fixing!
Looks great, small comments!
Jul 26 2023
Jul 25 2023
LGTM! Feel free to add the comment about the implications of using the flag in the docs.
Do we want to keep the experimental word in the flag?
Jul 24 2023
Please squash into previous patch, I see no reason to make them into separate commits. The first one is missing Release Notes, for example.
The review is marked as accepted, should we land it? Let me know if you need help with that :)
Jul 23 2023
A thought came to mind - since we are doing workarounds anyway, would it be easier to ask people to simply add -clang-diagnostic* to the Checks in their config file? It's fair to assume they will get those warnings when compiling the code. I feel the more workarounds we add in the code the harder it will be to clean it up later :)
Jul 22 2023
I have opened a refactoring ticket here: https://github.com/llvm/llvm-project/issues/64037
Jul 21 2023
I think it'd be good to add reviewers there
Jul 20 2023
This should be a configuration option, we should not hardcore project-specific things in the source code.
Jul 19 2023
Using hasSimpleCopyConstructor and so on greatly simplifies the logic, great! Let me know if you are happy with it or I should go ahead and merge.
Address review comments
Jul 18 2023
LGTM, thanks for the fix!
Split getInfo function into 2 functions, remove
Remove extra newline
Fix typo in release notes.
Remove confusing comment.
Jul 17 2023
Jul 16 2023
LGTM, thank you for the fix!
Looks good, great to see all these issues fixed! Have a couple small comments.
May 9 2023
Thanks for the review! Fixed your comments before landing.
May 7 2023
Separating the changes into two patches would only prolong this process and potentially delay the completion of this task.
A lot of our test files uses macros to differentiate between specific C++ standards, why not do that here too?
May 6 2023
Revert unwanted change to UseColor
- Add test where both command line and config settings are used.
- Document that the command-line option overrides the config option, for consistency with other command-line options.
May 5 2023
May 4 2023
Update commit message with Github issue.
Apr 23 2023
Thanks for the refactoring, great to remove some code duplication!
The commit message doesn't really tell me "what" this commit is fixing, it only points to a section of the Standard. It talks about "a false positive" but it doesn't tell what this FP is about. Could you write a little bit more about what the problem is? Preferably if you can link a Github issue describing the problem. The subject of the commit message should indicate which particular check it relates to.
I would agree that the fact that a function throws or not can only be found out when analyzing the function definition (it's impossible to know from the declaration). There's also a duplicate diagnostic that I don't see much value on, so I would agree with this patch unless I'm missing some concrete use case I'm not aware of.
Apr 20 2023
I believe this should be discussed in an RFC. We already have the standalone include-cleaner tool, why is that not sufficient? Can it be extended instead? There's also the include-what-you-use tool out there.
Apr 19 2023
First of all thank you for the contribution! I had just a quick look so here's some very preliminary comments, will have more time to deeply review during the weekend: