Merge from main.

Improve null checking, use the correct type in the fixit, and updated the tests.

I would like to see this merged in. Is there anyone available to review or approve?

@njames93 I fixed the review comments, can this be merged?

All review comments have been addressed. I assume this can be merged now.

So can this be merged now?

Anymore feedback?

Updated to diagnose return statements as well.

Update review comments and added fixit hints.

And for context, here is an actual bug this check would help find:

What is the motivation for requiring these to be the arguments of call or construct expressions?

Anymore feedback? Can this be merged now?

Fix typo and merge from main.

Thanks for merging as I dont have commit access.

What's the expected HD property of this template function clip?

Now, back to the specifics of your example. I'm still not 100% sure I understand what the problem is. Can you boil down the use case to an example on godbolt?

Could you give an example to demonstrate current use and how it will break?

A slight variation on that, that might be better: lambdas with no lambda-capture are implicitly HD; lambdas with any lambda-capture (which must therefore have an enclosing function) inherit the enclosing function's HDness.

Is it possible to add a test like this?

It captures addresses as seen at the point in time on the processor which executed the constructor.

I.e. if I pass a mutable lambda by reference to the GPU kernel

Not only the capture is an issue, like a regular function, lambda could also access non-local variables/functions.

says we capture host variable reference in a device lambda.

This needs a test when calling in a constexpr function. I believe std::invoke is not constepxr, so a function object call in a constexpr function should not suggest std::invoke.

Rebased

Is someone able to merge in my changes?

Some changes based on feedback.

In this sense bug reports against abandoned alpha checkers (which are, unfortunately, the majority) aren't very useful. In most cases it's just too easy to see how broken they are.

Moved flag for alpha checks to the ClangTidyContext

There doesn't seem to be a simple way to remove it from the ClangTidyOptions class, as a lot more functions need to be changed to support that. I would prefer to leave it there for now, and we can refactor it later. Either way, the check can't be set from a config file.

Rename flag to AllowEnablingAnalyzerAlphaCheckers.

When something is merged into Clang trunk, the expectation is that it will be production quality or will be worked on rapidly to get it to production quality, which is somewhat orthogonal to getting user feedback. I don't know that I have the full history of all of the alpha checks so my generalization may be inaccurate, but it seems like some of these checks were accepted as a WIP and the "progress" stopped for a long time with no one volunteering to remove the features from the analyzer.

Rename flag to allow-enabling-alpha-checks and removed the option from the clang-tidy file.

I think the premise is a bit off the mark. It's not that these are not for the common user -- it's that they're simply not ready for users at all.

But still, could you explain the use case and why a local modification of clang-tidy is not an option?

As Devin says (and as we discussed this with Anna Zaks) alpha checkers are still in development, so we don't want to expose them to the users, even very curious ones.

Do you have some better choices?

Do you want the flag to be renamed?

Am i understanding correctly that the new -enable-alpha-checks flag doesn't enable alpha checks, but it only allows enabling them with a separate command?

I'm curious about the use case for this, since I don't think it ever makes sense to run all the alpha checks at once. These checks are typically a work in progress (they're not finished yet) and often have false positives that haven't been fixed yet.

So I ran this on clang/llvm code base and fixed some false positives.

So moving to core will require explicit casts in some of the tests, especially for things like: memcpy(a262.s1, input, -1). Or this could be moved to another section instead of core. In D45532 there is talk of adding a bugprone section. I think this would be good there.

Have you tried on any large codebases?

While I understand extending the analyzer to cover more is a good approach, there is -Wconversion which seemingly covers this -- or at least the trivial case(?):

So I submitted my change to the ConversionChecker, instead.

Isn't this case already covered by conversion checker?

This seems like it would be nice to have in bugprone-* as well.

I have rebased.

Is someone able to merge in my changes?

Do you need someone to submit this for you?

So, I've added tests for class enums and bols.

As this patch can catch some mistakes, I'm fine with checking it in. I agree that it is reasonable to write normal code like sizeof(func_call()) (not false positive), maybe set the option to false by default?

I have reworded the documentation. Hopefully it is clearer.

So I've updated the documentation on this. Are there any other updates needed for this to get it merged in?

I don't have a script for it. I've used "bear" with at least some of those projects because they use makefiles rather than cmake (https://github.com/rizsotto/Bear). I'm not tied to those projects specifically, either, so if you have a different corpus you'd prefer to use, that's fine. The important bit is testing it across a considerable amount of C code and C++ code to see whether this diagnostic is too chatty or not.

Again, that only works for C++ and not C.

If the return type of foo() is changed, then the use for s1 will be automatically updated

Can you point to some real world code that would benefit from this check?

How does this differ from misc-unused-raii?

Can you elaborate a bit more about this?

The difference is, the current diagnostics allow the user to figure out what
happened. This change does not -- there's no way to find out which 'f' caused
the problem, and how we got from 'f' to 'g'.

gives the user no idea of how we got from a call to f into a call to g.

Added tests