In D45835#1072557, @efriedma wrote:

The list of features/extensions seems okay; it's information which is already available through a stable interface (specifically, clang -E). JSON also seems fine as a representation.

Including the language/codegen/etc. options as-is doesn't make sense; we can, and often do, change the internal representation of the option structs. If the information is useful, we should provide a stable interface.

Have you run this over any large code bases to see whether the check triggers in practice?

In D41648#1047799, @JonasToth wrote:

I checked several codebases and implemented a little helper script to see which kind of macros exist. Then I added some filter regex as configuration and tried again, here are the results:

For https://github.com/nlohmann/json which is a very high quality codebase, the results were as expected: very clean, and good macro usage: (except a #define private public for tests)

Filter: JSON*|NLOHMANN*

all_macros.txt695 B

Download

filtered_macros.txt47 B

Download

CMake on the other hand uses many macros and don't obey CAPS_ONLY rules for all macros.

Filter: _FILE_OFFSET_BITS|AE_*|ARCHIVE_*|cal_*|CMAKE_*|cm*|CM_*|CPP_*|CURL*|E_*|exp_*|F90*|jp*|JSON_*|KW*|kw*|O_*|REQ_*|UV_*|YY*|yy*|Z_*

Committed in r330188, thank you for the patch!

LGTM! I will go ahead and commit on your behalf. Btw, if you're thinking of making additional patches, now might be a good time to ask for commit privileges (https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access).

I've committed in r330185, thank you for the patch!

Committed in r330159. Thank you for the patch!

LGTM!

LGTM aside from a few small nits.

In D45615#1066975, @paulsemel wrote:

In D45615#1066973, @lebedev.ri wrote:

Tests?

I can do this. But currently, I am not testing the formats in the tests..

I'd also be interested to see the number of false positives and true positives when run over some large code bases (both C and C++). For instance, I would imagine code like this to be somewhat common and very reasonable:

void some_func(int someArg, bool *someResult) {
  if (someResult)
    *someResult = false;
}

Especially in C code where references are not available. This makes me wary of making this a compiler diagnostic, but clang-tidy may still be a reasonable place for this functionality to live.

Committed the fix in r329930.

I think this code is ready to go in -- accepting my own revision so that I can close. If @delesley spots any issues, they can be address post commit.

I've commit in r329904, thank you for the patch!

Aside from some minor nits, LGTM!

I see we all found the solution at the same time. Yay teamwork! :-D

In D44093#1063658, @paulsemel wrote:

Sorry about it, I also have the warning on my machine, but not the error you get...
Those test are actually working on my different linux machines, that's really weird.
This one is actually really weird, because I could find manually the missing pattern in your output.. I just don't get what is happening.

Thank you for rebasing, but the patch does not pass tests locally for me. I get the following results (testing on Windows x64 with MSVC 2017 in a Debug build):

Thank you for working on this!

LGTM!

Can you rebase the patch? The patch did not apply cleanly for me against trunk (the contents of the check function are appearing in CheckARMBuiltinExclusiveCall()).

Ping.

Ping.

In D44093#1063340, @paulsemel wrote:

I don't really know what's the procedure right now.. What should I do once you both accepted the patch ? :)

LG aside from a diagnostic wording nit.

LGTM with some minor commenting nits.

LGTM aside from a minor docs nit.

I don't think we're currently diagnosing static data members of classes as being unused in the first place; are there plans to implement that functionality so that someone might want to write the attribute there?

LGTM with a few minor nits to be fixed.

Sorry for the delayed review; I'm back from vacation now and am picking things up again.

Adding some folks to review the dynamic matcher bits.

LGTM with a minor doc nit.

In D44559#1044639, @rjmccall wrote:

In D44559#1044186, @avt77 wrote:

In D44559#1040799, @rjmccall wrote:

I think we're correct not to warn here and that GCC/ICC are being noisy. The existence of a temporary promotion to a wider type doesn't justify warning on arithmetic between two operands that are the same size as the ultimate result. It is totally fair for users to think of this operation as being "closed" on the original type.

Could you please clarify, are you saying that PR35409 is not a bug, and clang should continue to not warn in those cases?

Correct.

Does it mean we should abandon this revision? On the other hand it's a real bug, isn't it?

Not as I see it, no.

LGTM!

This generally looks reasonable to me, but @rsmith should weigh in before you commit because MultiSourceLocation is novel.

I think hasSourceExpression() already does what you're looking for.

void f() {
  int i = 1.0f;
}

clang-query> match implicitCastExpr(hasSourceExpression(floatLiteral()))

You should also regenerate the AST matcher documentation by running docs/tools/dump_ast_matchers.py

LGTM!

I don't know enough about ObjC to feel comfortable reviewing this, but I've added @rjmccall who may have opinions.

In D44155#1041584, @lebedev.ri wrote:

Please upload patches with full context (-U99999) :)

Now: with more context!

Ping.

This looks reasonable to me, but I'm also not a python expert.

In D44559#1040435, @craig.topper wrote:

Fair point, what is the default signedness of char?

In D44559#1040398, @craig.topper wrote:

gcc also warns for this

short foo(char a) {

return a * a;

}

Despite the fact that the char would be promoted to int, the upper bits are just sign bits and the multiply result still fits in a short.

In D44231#1039888, @hokein wrote:

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?

LGTM with some minor documentation nits.

LGTM!

Adding Richard as a reviewer, because this seems somewhat clever to me (so there may be a better change to make).

LGTM aside from a testing nit.

In D44439#1036868, @ilya-biryukov wrote:

@aaron.ballman , here's a test-case I came up with. I'll fix the other issue (invalid error, that forces the test to fail) and submit it for review along with the fix for the error. Does that SG?

LGTM, thank you for the explanation about the tests.

Test case for this change?

In D16008#1035948, @malcolm.parsons wrote:

In D16008#1035789, @aaron.ballman wrote:

Do you know why the CSA checker is still in alpha?

It isn't - https://reviews.llvm.org/D26768 moved it to optin.

I don't know why https://reviews.llvm.org/D34275 didn't turn it on by default.

I dreamed up some test cases that may or may not make sense. I think they boil down to a few things:

In D43248#1036409, @jdenny wrote:

I'd prefer to move it than to expect people to obey such a comment. Let's see what Aaron says.

In D16008#1035683, @alexfh wrote:

If the CSA checker is still in alpha, I'd proceed with this check instead of investing time in polishing the CSA implementation.

LGTM with the test comments fixed up.