Daniel is a better reviewer here than myself. A few cents from me though: why do we want to make an exception for comments and not for regular code?

LG. Looks like a pretty mechanical change.

Sorry for the delay, I was on vacation.

LG with one nit.

LG

LG. Thank you for the fix!

In D34002#776948, @chh wrote:

Thanks for providing the ODR reason for Android -fno-exceptions users to change source code or suppress this warning themselves.

In D34002#776551, @chh wrote:

Android source is suppressing misc-noexcept-move-constructor warnings
because -fno-exceptions is used and Android does not like to add more
exception specific code.

In D34002#776193, @chh wrote:

In D34002#775830, @alexfh wrote:

IIUC, when vector<T> (for a class T that has both move and copy constructors) resizes, it will prefer move constructors, but only if they're declared noexcept. This is true even if -fno-exceptions is on. So I don't think this check should depend on -fno-exceptions.

Should the compiler assume noexcept when -fno-exceptions is on?
That means move constructors should be preferred under -fno-exceptions, and this check would be unnecessary, right?

In D20693#776030, @hintonda wrote:

Great, thanks for you help.

Could you commit this for me?

In D33365#775880, @lebedev.ri wrote:

In D33365#775860, @alexfh wrote:

I guess, this check should go to readability or elsewhere, but definitely not to misc.

Hmm, misc may be a bad place for this, but i think readability is even worse fit.
The best guess would be something like hardening / security, but there is no such category.

I guess, this check should go to readability or elsewhere, but definitely not to misc.

I would be interested in seeing the results of this check's run on LLVM+Clang code.

Here's a small test: https://godbolt.org/g/nNZgUb (look for T::T( and S::S( calls, adding -fno-exceptions doesn't change which constructors are called).

IIUC, when vector<T> (for a class T that has both move and copy constructors) resizes, it will prefer move constructors, but only if they're declared noexcept. This is true even if -fno-exceptions is on. So I don't think this check should depend on -fno-exceptions.

Now that you found the root cause, this looks like a proper fix ;)

In D20693#775197, @hintonda wrote:

I have not, as yet, been able to reproduce the buildbot failures. They were essentially intermittent seg-faults, and corrupt diag output.

I will work on creating a test that can reproduce the problem.

In D20693#775153, @alexfh wrote:

In D20693#775014, @hintonda wrote:

• Only pass %2 parameter if %2 is included in format.

I thought, DiagnosticsBuilder handles placeholders in conditional parts correctly. Did you find an evidence of the opposite? Can you add a test that consistently fails?

In D20693#775014, @hintonda wrote:

• Only pass %2 parameter if %2 is included in format.

LG

In any case, I'm strongly against these hacks, please revert them.

In D20693#774693, @hintonda wrote:

In order to fix diagnostic corruption in some of the buildbot tests
(unable to reproduce locally):

• make NoexceptMacro a static variable so it's lifetime doesn't end when UseNoexceptCheck is destroyed.
• pass a const char* instead of a StringRef to DiagnosticBuilder so it won't create a temporary std::string and cache the address of the temporary char * it owns.

LG. Thank you!

Looks good!

In D33304#771215, @hokein wrote:

Thanks for the contributions.

All your three checks are not android specific -- because they are checking POSIX APIs (open, creat, fopen), which are more likely related to POSIX. So personally, I'm +1 on a "posix" module, instead of "android", but wait to see other reviewers' opinions before renaming it.

LG

Please add a test for the new -style syntax.

In D33304#768961, @yawanng wrote:

In D33304#768731, @alexfh wrote:

It's awesome to see another two checks, and it seems to justify adding a separate module, but I'd prefer the other two checks to be sent for review as separate patches to make the review easier and faster.

The other two checks are relatively small and simple. I understand this may make the review process longer and we could wait for that. Please take your time :-) Thank you!

It's awesome to see another two checks, and it seems to justify adding a separate module, but I'd prefer the other two checks to be sent for review as separate patches to make the review easier and faster.

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.

Sam knows this stuff much better.

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

The check ignores returns of const pointers (meaning * const, not const *); while pointless, these are not particularly harmful. It could be made laxer by ignoring all trivially copyable types (on the grounds that they won't have interesting move constructors/assigners anyway), or stricter by ignoring all const returns (on the grounds that, in the best case, it's just pointless verbosity). Or it could be made an option.

Thanks, LG!

Thank you! This looks good with one nit (see the inline comment).

IIUC, there was no consensus about usefulness of this change.

IIUC, there was no consensus about usefulness of this change.

IIUC, there was no consensus about usefulness of this change.

I'm going to commit the patch for you, but I guess, you might want to ask for commit access (http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access).

LG. Thank you for investigating this!

I'll repeat my comment from D25558: "I'm thoroughly confused as to why the code in your test was not handled correctly and why this is the right fix. Can you explain?" The "For some reason ..." part doesn't really explain anything. I guess, we're papering over a more generic problem, and someone has to figure out what it is and how to fix it properly.

LG. Thank you!

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.

The existing google-runtime-memset-zero-length check is related. It finds cases when the byte count parameter is zero and offers to swap that with the fill value argument. Perhaps the two could be merged while maintaining backward compatibility through an alias.

LG

Sorry, missed your patch somehow. LG with one nit.

LG once the comments are addressed.

Instead of handling LinkageSpecDecl, use getRedeclContext() when issuing -Wshadow diagnostic.