Aside from a few small nits with the test cases, this LGTM! You should hold off on committing for a day or two in case Richard or others have comments on the patch.

A few more nits, but this feels like it's getting close to ready (at least, to me).

This is generally looking good to me, with a few small nits. @rsmith, do you have thought on this?

Ping

Committed in r305432. You should consider reaching out to Chris Lattner for svn access!

LGTM!

In D33904#778165, @george.burgess.iv wrote:

Why not just use __has_feature(overloadable_unmarked) or similar?

My impression was that __has_feature was was for larger features than tweaks to attributes. If this would be an appropriate use of __has_feature, though, I'm happy to keep things simple.

I'll update the other review with __has_feature. If it goes in with that, I'll abandon this.

In rL305025#101654, @Lekensteyn wrote:

It turns out that these tests are failing because:

• 1.2f == 1.2 is false, apparently 1.2 cannot be exactly represented as single-precision float. Without modifying the grammar, one can use floatLiteral(equals(1.2000000476837158203125)) to match 1.2f.

Adding Richard to the review for some wider perspective than just mine on the overall design.

LGTM!

LGTM!

Aside from one minor nit, LGTM!

Aside from one minor nit, LGTM

Aside from a minor suggestion, LGTM!

This check is an interesting one. The rules around what is signal safe are changing for C++17 to be a bit more lenient than what the rules are for C++14. CERT's rule is written against C++14, and so the current behavior matches the rule wording. However, the *intent* of the rule is to ensure that only signal-safe functionality is used from a signal handler, and so from that perspective, I can imagine a user compiling for C++17 to want the relaxed rules to still comply with CERT's wording. What do you think?

Thank you for working on this. How does this check compare with the -Wcast-align diagnostic in the Clang frontend? I believe that warning already covers these cases, so I'm wondering what extra value is added with this check?

In D33788#771671, @bruno wrote:

I'm unaware of any other API that canonicalizes the path, which is what users of this API are going to expect.

You can also call sys::path::remove_dots(Path, /*remove_dot_dot=*/true);

In D33788#771504, @akyrtzi wrote:

Getting the real path is notoriously slow (at least it's horrible in OSX, not sure about linux). Since this is about dropping the '/../' part, could we do some simple canonicalization of removing the dots ? Not sure if there is an existing function that does that.

In D33537#771159, @baloghadamsoftware wrote:

In D33537#770264, @aaron.ballman wrote:

I think we should try to get as much of this functionality in D33333 as possible; there is a considerable amount of overlap between that functionality and this functionality. This check can then cover only the parts that are not reasonably handled by the frontend check instead of duplicating diagnostics the user already receives.

I suppose that the frontend check will not travarse the call-graph just check direct throws. Should we only check indirect throws then?

In D33788#771070, @bruno wrote:

Hi Aaron,

Nice catch! Any chance you can add a testcase to this?

In D33735#770296, @ABataev wrote:

In D33735#770288, @aaron.ballman wrote:

Can you help me to understand what problem is being solved with this new attribute? Under what circumstances would the first argument be an ImplicitParamDecl but not an implicit this or self?

For captured regions an outlined function is created, where all parameters are ImplicitParamDecls. And the very first parameter is wrongly treated as 'this' argument of the member function.

Can you help me to understand what problem is being solved with this new attribute? Under what circumstances would the first argument be an ImplicitParamDecl but not an implicit this or self?

In D33537#765445, @baloghadamsoftware wrote:

In D33537#764834, @Prazek wrote:

How is that compared to https://reviews.llvm.org/D19201 and the clang patch mentioned in this patch?

Actually, this check does much more. It does not only check for noexcept (and throw()) functions, but also for destructors, move constructors, move assignments, the main() function, swap() functions and also functions given as option. A more important difference is that we traverse the whole call-chain and check all the throw statements and try-catch blocks so indirectly throwing functions are also reported and no flase positives are caused by throw and catch in the same try block.

In D33333#768332, @jyu2 wrote:

Okay this CFG version of this change. In this change I am basic using same algorithm with -Winfinite-recursion.

In addition to my original implementation, I add handler type checking which basic using https://reviews.llvm.org/D19201 method.

Addressing review comments.

Can you run clang-format over both the test files? Aside from that, looks good to me, but you should wait for @rsmith or @majnemer to sign off before committing.

Can you add some unit tests?

In D33531#767640, @alexfh wrote:

In D33531#767628, @aaron.ballman wrote:

In D33531#767059, @alexfh wrote:

Would it make sense to silence this diagnostic in the presence of also checking for cert-dcl21-cpp for such operators?

Currently there's no mechanism in clang-tidy to express dependencies or compatibility issues between checks. Moreover, we already have checks that are not meant to be run together, for example, readability-braces-around-statements and its google- incarnation (and other alias checks with different settings). That said, we could whitelist postfix increment and decrement operators in this check. Camillo, WDYT?

I can imagine a generic whitelist mechanism might be useful for this check. It could even be empty by default, but the documentation could call out cert-dcl21-cpp specifically and show an example of how you can run both checks.

A generic whitelist of method/function names would make sense, if we had more use cases for it. It might also be quite tricky to implement: distinguishing between prefix and postfix increment/decrement operators would require specifying arguments, and allowing it for all types would need a support for pattern matching or optional omission of the type name on methods. All this seems to be an overkill so far.

In D33531#767059, @alexfh wrote:

Would it make sense to silence this diagnostic in the presence of also checking for cert-dcl21-cpp for such operators?

Currently there's no mechanism in clang-tidy to express dependencies or compatibility issues between checks. Moreover, we already have checks that are not meant to be run together, for example, readability-braces-around-statements and its google- incarnation (and other alias checks with different settings). That said, we could whitelist postfix increment and decrement operators in this check. Camillo, WDYT?

I'm not opposed to this check, but I'm not keen on having a check that directly contradicts the output from another check. The cert-dcl21-cpp check will diagnose user's code when a postfix operator ++ or -- is *not* const-qualified. Would it make sense to silence this diagnostic in the presence of also checking for cert-dcl21-cpp for such operators?

Commit in r303913

Commit in r303912

In D33546#764842, @zturner wrote:

It might be overkill, but CMAKE_HOST_SYSTEM_PROCESSOR probably contains what you need. In any case, lgtm even without doing that.

Once you fix the typo in the check, can you run it over some large C++ code bases to see if it finds any results?

Now, with CMake warning. This is only needed for the llvm cmake files (since Clang relies on them anyway).

In D33546#764526, @zturner wrote:

In D33546#764521, @aaron.ballman wrote:

In D33546#764507, @zturner wrote:

Can we just error out if this is not specified and have CMake print the error message? CMAKE_GENERATOR_TOOLSET contains the value at generation time, and we can just check the value and print an error. Given that you can't successfully build LLVM / clang / etc without it, we might as well fail fast.

We can successfully build LLVM/clang/etc without it (I've done so for *far* too long), but you may get some sporadic "linker out of memory" errors (recompiles will eventually link, however). Also, not everyone may want to use the native toolset. I'm thinking about people who may have something like 8GB of RAM, where the x86 linker is less likely to throttle the machine than the x64 linker.

Since it does work out of the box, why force users to use an extra command line argument?

I've never successfully built clang without using the x64 host toolchain. At the very least, I think we should print a warning.

In D33546#764507, @zturner wrote:

Can we just error out if this is not specified and have CMake print the error message? CMAKE_GENERATOR_TOOLSET contains the value at generation time, and we can just check the value and print an error. Given that you can't successfully build LLVM / clang / etc without it, we might as well fail fast.

LGTM!

LGTM!

In D33333#761224, @rnk wrote:

Re: clang-tidy, I would rather implement this as a traditional compiler warning.

In D33333#761208, @aaron.ballman wrote:

In D33333#761126, @jyu2 wrote:

As I said, I don't think checking throw type matching catch handler type is compiler's job. I'd like either silence all warning for that. What do you think?

Consider the following cases:
...
I think the expected behavior in these cases is reasonable (and can be done with a CFG) rather than manually looking at the scope stack like you're doing.

I agree with @jyu2, we should do something simple. I think it would be simple to handle all cases except for h, which requires semantic analysis to figure out if the thrown object would be caught by the catch parameter. Implementing that is more likely to introduce bugs in the compiler than it is to find bugs in user code.

In D33333#761126, @jyu2 wrote:

In D33333#760431, @aaron.ballman wrote:

In D33333#760419, @jyu2 wrote:

In D33333#760149, @aaron.ballman wrote:

As an FYI, there is a related check currently under development in clang-tidy; we probably should not duplicate this functionality in both places. See https://reviews.llvm.org/D19201 for the other review.

To my understanding, clang-tidy is kind of source check tool. It does more intensive checking than compiler.
My purpose here is to emit minimum warning form compiler, in case, clang-tidy is not used.

Sort of correct. clang-tidy doesn't necessarily do more intensive checking than the compiler. It's more an AST matching source checking tool for checks that are either expensive or have a higher false-positive rate than we'd want to see in the frontend.

I think that this particular check should probably be in the frontend, like you're doing. My biggest concern is that we do not duplicate functionality between the two tools. D19201 has has a considerable amount of review, so you should look at the test cases there and ensure you are handling them (including the fixits). Hopefully your patch ends up covering all of the functionality in the clang-tidy patch.

In D33333#760353, @Prazek wrote:

Could you add similar tests as the ones that Stanislaw provied in his patch?
Like the one with checking if throw is catched, or the conditional noexcept (by a macro, etc)

Good idea! Could add “marco” test for this. But I am not sure compiler want to add check for throw caught by different catch handler. Because at compile time, compiler can not statically determine which catch handler will be used to caught the exception on all time. I would think that is pragma's responsibility.

For example:

If (a) throw A else throw B;

My main concern there is implicit noexcept gets set by compiler, which cause runtime to termination.

As I said, I don't think checking throw type matching catch handler type is compiler's job. I'd like either silence all warning for that. What do you think?

In D33333#760419, @jyu2 wrote:

In D33333#760149, @aaron.ballman wrote:

As an FYI, there is a related check currently under development in clang-tidy; we probably should not duplicate this functionality in both places. See https://reviews.llvm.org/D19201 for the other review.

To my understanding, clang-tidy is kind of source check tool. It does more intensive checking than compiler.
My purpose here is to emit minimum warning form compiler, in case, clang-tidy is not used.

In D33135#754278, @Lekensteyn wrote:

By the way, I think that long double is less common than long unsigned literals, so changing unsigned to uint64_t might be something more important?

In D19201#760358, @Prazek wrote:

In D19201#760151, @aaron.ballman wrote:

As an FYI, there is a related check being implemented in clang currently; we probably should not duplicate this effort. See https://reviews.llvm.org/D33333.

I think that clang is the right place for this feature, but I am not sure if the patch has all the features (like checking if something will be catched, or not showing warning for conditional noexcepts, which as we have seen
could be a problem for some projects. There also might be some other corner cases that we didn't think about.
Assuming that this patch is ready to land, I would propose to send it to trunk and remove it when the clang's patch will make it to the trunk. I am not sure how much time it will take for other patch to be ready, but maybe we could gather some usefull bug reports in the meantime and also Stanislaw would have a contribution.

As an FYI, there is a related check being implemented in clang currently; we probably should not duplicate this effort. See https://reviews.llvm.org/D33333.

As an FYI, there is a related check currently under development in clang-tidy; we probably should not duplicate this functionality in both places. See https://reviews.llvm.org/D19201 for the other review.

In D33304#758808, @alexfh wrote:

In D33304#758713, @aaron.ballman wrote:

In D33304#758624, @srhines wrote:

In D33304#758621, @joerg wrote:

I find the use of "must" at the very least inappropriate. If there was no use case for not including it, it wouldn't be an option. There is also nothing really Android-specific here beside maybe the open64 mess.

On Android, we are requiring this flag. That is why this is part of a new category of Android-specific tidy rules. If you think this belongs more generally in a different category for tidy, can you suggest somewhere else to put it? We didn't want to impose these restrictions for platforms that might not want to be so strict. Also, as with any static analysis, there is the possibility that the original code author intended to "break" the rules, but that is what NOLINT is for.

I'm not keen on putting this in an Android module either, as it's not really Android-specific behavior. For instance, this is also part of a recommended compliant solution for CERT FIO22-C.

I think AOSP has enough specific guidelines and requirements to warrant a separate module (especially, if Android folks have plans to contribute more than one check into it ;). As for this check, if the relevant requirements of CERT and Android are really identical, we could make an alias for the check in the CERT module (or vice versa). Another possibility that comes to mind is to create a new "posix" module specifically for things related to POSIX APIs (or "unix", if we want it to be slightly broader). WDYT?

In D33304#758624, @srhines wrote:

In D33304#758621, @joerg wrote:

I find the use of "must" at the very least inappropriate. If there was no use case for not including it, it wouldn't be an option. There is also nothing really Android-specific here beside maybe the open64 mess.

On Android, we are requiring this flag. That is why this is part of a new category of Android-specific tidy rules. If you think this belongs more generally in a different category for tidy, can you suggest somewhere else to put it? We didn't want to impose these restrictions for platforms that might not want to be so strict. Also, as with any static analysis, there is the possibility that the original code author intended to "break" the rules, but that is what NOLINT is for.

In D32332#751785, @george.burgess.iv wrote:

I'd be happy with that approach. Do you like it, Aaron?

LGTM!

Please be sure to regenerate the AST matcher documentation as well by running clang/docs/tools/dump_ast_matchers.py

In D32942#751338, @lebedev.ri wrote:

In D32942#751324, @aaron.ballman wrote:

LGTM!

Thanks.
I guess we want to wait a bit more for @alexfh to leave any comment?

LGTM!

This looks reasonable to me.

LGTM!

Have you tried running this over a large code base to see how frequently the diagnostic triggers? I'm wondering why 10 was chosen as the default threshold.

Committed in r302419, thank you!

LGTM, thank you! Do you need me to commit this for you?

LGTM!

Thanks! I've commit in r302275.

LGTM!

LGTM!

LGTM!

Thank you for working on this check!