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)

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.

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?

LGTM, thanks for fixing!

LGTM!

LGTM!

LGTM

LGTM!

Do you have plans to also detect the bugprone scenario described in the Notes here?

In D157181#4563170, @PiotrZSL wrote:

In D157181#4563147, @carlosgalvezp wrote:

Strange, it seems the alias was never fully added?
https://reviews.llvm.org/D117522

Looks like, still I dont know what to do with release notes, add alias "again" there, dont change it, or mention that there were bug.

LGTM, minor question that can be fixed post-review unless you want to discuss further!

LGTM!

LGTM

In D145138#4561799, @PiotrZSL wrote:

Test are failing, and I do not think that converting c-strings into std::array is a good idea....

Strange, it seems the alias was never fully added?
https://reviews.llvm.org/D117522

LGTM!

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

LGTM, thanks for fixing!

Looks great, small comments!

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?

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 :)

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 :)

I have opened a refactoring ticket here: https://github.com/llvm/llvm-project/issues/64037

LGTM!

I think it'd be good to add reviewers there

In D155890#4523262, @gribozavr2 wrote:

In D155890#4523243, @adukeman wrote:

In D155890#4522266, @ymandel wrote:

In D155890#4521266, @carlosgalvezp wrote:

This should be a configuration option, we should not hardcore project-specific things in the source code.

I agree, but we already are hardcoding specific types -- I think this is a separate (and valid) critique of the design. I'd propose filing an issue on the github tracker and we can follow up there. I, for one, would love to review such a change but don't have the time to write it.

Is moving these values to config an appropriate task for somebody like me new to working on clang-tidy? I'd be happy to merge this and then try the transition to a config assuming there's some similar examples I can borrow from elsewhere in the codebase.

I think it can be a good starter task for a new engineer on the project. However, don't underestimate this problem, it will require the code to be refactored a little bit. For example, the function hasOptionalClassName needs restructuring so that it can accept class names from a list. Not a lot of work, but it isn't mechanically replacing string literals with a variable either.

This should be a configuration option, we should not hardcore project-specific things in the source code.

Merge matchers.

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

In D155625#4512123, @PiotrZSL wrote:

LGTM, but I'm not sure if isCopyableOrMovable will always work correctly, for a user defined special members it looks ok, but for some implicit ones I worry it may not always work. Probably things like "(hasSimpleCopyAssigment()) || (hasUserDeclaredCopyAssigment() && check here if its not deleted)" would be needed.
Thing is that CXXRecordDecl got most info (in confused way), there are things like DefaultedMoveConstructorIsDeleted that could be used to verify somehow base class.

LGTM, thanks for the fix!

Split getInfo function into 2 functions, remove
structured bindings.

Remove extra newline

Fix typo in release notes.

Remove confusing comment.

LGTM, thanks!

LGTM, thank you for the fix!

Looks good, great to see all these issues fixed! Have a couple small comments.

Thanks for the review! Fixed your comments before landing.

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?

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.

Update commit message with Github issue.

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.

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.

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: