LGTM!

LGTM!

LGTM, thank you for this!

The review says you abandoned it -- was that accidental?

LGTM.

Thank you for working on this check!

Mostly LGTM but a few nits.

LGTM! Thank you for this refactoring -- it was large, but I think the results from it are really good.

Aside from a minor nit, this LGTM. However, I'm not the most familiar with how cross-compiling works in the first place, so I may be the wrong one to approve this.

In D66856#1650803, @aaron.ballman wrote:

To me, actual problems at runtime belong in -Wformat and logical inconsistencies that don't cause runtime problems belong in -Wformat-pedantic. However, I think it's a defect that the C standard has no clearly UB-free way to print a _Bool value. I will bring this up on the reflectors to ask if we can clarify the standard here.

Thank you for doing all this work -- in general, this is looking great and really cleans things up!

In D67140#1664406, @gribozavr wrote:

In D67140#1664106, @NoQ wrote:

In D67140#1659982, @gribozavr wrote:

We should take a page from desktop software here. If the messages were in a separate file, there would be a lot of people capable of mass-editing them. When messages are hardcoded in the tool code, navigating and editing them requires more skill, and definitely a lot more jumping around.

In the Static Analyzer there's often an explosive amount of dynamically generated messages that are going to be pretty hard to stuff into a tablegen pattern. Say, you can probably turn this into "%0 %1 %2 with a %3 retain count into an out parameter %4%5" but would it really help?

Unfortunately, that message is already not following best practices. One should not be passing snippets in natural language into substitutions. Every character that is a natural language construct should be in the message string. The only allowed substitutions are types, names, numbers and such. We already support %select in message strings, but there are frameworks that are more flexible -- we can port features from them into our message strings as needed.

LGTM!

In D64644#1661417, @Mordante wrote:

Thanks for the review.
Could you commit this patch? I don't have commit access.

LGTM!

Aside from the missing documentation bit, I think this LG.

In D67140#1659978, @Szelethus wrote:

In D67140#1659907, @NoQ wrote:

In D67140#1659872, @aaron.ballman wrote:

If we're okay with the inconsistency but still feel like giving ourselves more work, adding proper punctuation to Static Analyzer diagnostics would at least make them grammatically correct. :-D

I'd love us some punctuation! Unfortunately the last time a person who actually speaks English committed actively to the Static Analyzer was, like, a couple of years ago at least (:

I guess there's a joke waiting to be made about a couple Hungarians and Russians discussing how to write proper English :^)

In D67140#1659831, @NoQ wrote:

In D67140#1659774, @aaron.ballman wrote:

I don't think it's a requirement (so long as the diagnostics are clear about the issue being diagnosed, I'm happy enough), but I think it's good for a tool to be self-consistent in its messaging. It's jarring that one part of the compiler emits diagnostics one way and another emits them a totally different way. It may be less of an impact for people who don't see the output from both at the same time, but that happens to be the use case I have.

Unfortunately i think at this point many clients who tried to integrate multiple tools resorted to "Automatic Message Recapitalization" (c). For that reason we probably can harmlessly decapitalize Static Analyzer messages. I suspect that it won't really change anything either, because most tools will still be afraid of accidental inconsistencies in the compiler.

In D67140#1659749, @gribozavr wrote:

In D67140#1659356, @aaron.ballman wrote:

In D67140#1658969, @gribozavr wrote:

In D67140#1658365, @aaron.ballman wrote:

Ah, good to know! That reduces my concern, but doesn't negate it. AFAIK, we haven't changed the interface such that it requires code changes rather than just a recompile in recent history, so this is a bit novel.

I think API changes happen all the time. At Google, we are integrating upstream LLVM and Clang changes into our internal codebase daily. We have a lot of internal ClangTidy checkers. Fixing up all our internal code to keep with upstream changes is a full time job for one engineer (but it is a rotation).

I think this sort of backs up the point I was making.

Sorry, but in the previous comment you were saying that "we haven't changed the interface such that it requires code changes". So which is it, for you?

In D54408#1659301, @Abpostelnicu wrote:

@aaron.ballman
I think the auto usage improves and simplifies the code, however, as I would really like to see this code landed, I would be ok to help Stephen to finish this work and replace auto by the "old" way. Do you think we can have this approved if we make the changes mentioned earlier?

In D67140#1658969, @gribozavr wrote:

In D67140#1658365, @aaron.ballman wrote:

Ah, good to know! That reduces my concern, but doesn't negate it. AFAIK, we haven't changed the interface such that it requires code changes rather than just a recompile in recent history, so this is a bit novel.

I think API changes happen all the time. At Google, we are integrating upstream LLVM and Clang changes into our internal codebase daily. We have a lot of internal ClangTidy checkers. Fixing up all our internal code to keep with upstream changes is a full time job for one engineer (but it is a rotation).

In D66919#1658483, @aaronpuchert wrote:

In D66919#1658355, @aaron.ballman wrote:

We do have numerous warnings that are default errors, you can look for DefaultError in the diagnostic .td files to see the uses.

Thanks, I hadn't seen that before. It seems that most of these warnings are for extensions, but one comes pretty close to what @dexonsmith has suggested:

def warn_cannot_pass_non_pod_arg_to_vararg : Warning<
  "cannot pass object of %select{non-POD|non-trivial}0 type %1 through variadic"
  " %select{function|block|method|constructor}2; call will abort at runtime">,
  InGroup<NonPODVarargs>, DefaultError;

The standard explicitly says in C11 6.5.2.2p8: “the number and types of arguments are not compared with those of the parameters in a function definition that does not include a function prototype declarator”, but perhaps this just means a programmer can't rely on such a check?

In D67159#1659103, @simon_tatham wrote:

Come to think of it, it would also not be too hard to constrain it to only be usable for a particular subset of builtins, and perhaps even only with a particular set of alias names for them. (I could easily derive all that information from the same Tablegen that arm_mve.h itself is made from.)

I share in the discomfort expressed for this attribute, but I don't have a better solution in mind just yet, so I'm giving some other review feedback in the meantime.

In D67140#1658353, @gribozavr wrote:

In D67140#1658315, @aaron.ballman wrote:

In D67140#1656831, @NoQ wrote:

Honestly, i'm much more worried about message capitalization :)

Likewise. I wish the static analyzer would follow the usual conventions followed by clang and clang-tidy. ;-)

I have the opposite opinion -- I wish that ClangTidy used complete sentences, and multiple sentences if it makes sense. The sentence fragments are too brief to explain complex and nuanced topics that ClangTidy communicates about. ClangTidy often plays the role of a developer education tool. It is not a guard like a compiler; developers can totally ignore ClangTidy if they disagree with the message. The better we can explain the problem, the more likely it is the developer will act on the message. I believe static analysis tools would be better off if we could write multiple sentences in the diagnostic.

Even for compiler messages, a sentence fragment is sometimes too concise.

In D66919#1655052, @aaronpuchert wrote:

In D66919#1655048, @dexonsmith wrote:

I went back to read notes from when we deployed -Wstrict-prototypes (which we have had on-by-default for our users for a couple of years, since it catches lots of -fblocks-related bugs). I was mainly objecting because I had (mis)remembered trying and backing out the specific behaviour you're adding. On the contrary, our notes say that we might want to strengthen the diagnostic as you're doing.

Thank you for having a look at the notes, that's good to hear.

Your users are Xcode/Apple Clang users? @aaron.ballman suggested to turn the warning on per default, and if Apple has that deployed already, it might be another argument in favor of doing so in vanilla Clang as well.

In D67023#1656453, @jfb wrote:

In D67023#1654704, @aaron.ballman wrote:

In D67023#1654070, @jfb wrote:

I refer you to http://wg21.link/p0883
I don’t think this diagnostic should be added to clang until p0883 is fully implemented, even just for C. Otherwise we leave users with no portable way to do the right thing without diagnostic.

P0883 has not been adopted, that I can tell (it has strong support, but isn't [expected to be] a part of C++2a)?

I think it'll make it in as an NB comment.

Also, this functionality is implemented by libc++ and libstdc++, so I'm not certain what you mean by "until p0883 is fully implemented" or why that paper would be implemented "even just for C". Are you suggesting to gate this deprecation for C functionality based on what C++ standard library implementations are doing?

Yes, because atomics are already hard enough to use correctly between C and C++.

In D67140#1656831, @NoQ wrote:

Honestly, i'm much more worried about message capitalization :)

In D67057#1658217, @piscisaureus wrote:

Fixed nit, this is ready to land IMO.

LGTM!

LGTM aside from a small nit. Thank you for this!

In D67122#1656205, @lebedev.ri wrote:

In D67122#1656189, @aaron.ballman wrote:

One fear I have with this is in expansions of the offsetof macro, where it is a common implementation strategy to cast a null pointer to be of the correct type when calculating member offsets. Do you think you will be able to distinguish between null pointer additions that the user wrote directly (which is UB) as opposed to null pointer additions that come from the implementation (which is not UB)?

Can you show a snippet on godbolt?

One fear I have with this is in expansions of the offsetof macro, where it is a common implementation strategy to cast a null pointer to be of the correct type when calculating member offsets. Do you think you will be able to distinguish between null pointer additions that the user wrote directly (which is UB) as opposed to null pointer additions that come from the implementation (which is not UB)?

In D67023#1654070, @jfb wrote:

I refer you to http://wg21.link/p0883
I don’t think this diagnostic should be added to clang until p0883 is fully implemented, even just for C. Otherwise we leave users with no portable way to do the right thing without diagnostic.

In D67023#1653425, @jfb wrote:

Is atomic initialization now correct in all modes (including C++) without this macro?

In D66397#1652771, @xbolva00 wrote:

tbh, I would appreciate if you would leave the definition of diagnoseXorMisusedAsPow() where it is and add a forward declare of the function earlier in the file.

I uploaded the patch almost 2 weeks ago and nobody complained so I thought it is okay. Uploaded, fixed.

LGTM, thank you for working on this!

LGTM aside from a nit with auto.

Generally LG to me as well

In D66850#1651519, @piscisaureus wrote:

@aaron.ballman
I was able to run the tests, I think this is good to go.
Can you help me get it landed?

In D66919#1651108, @dexonsmith wrote:

In D66919#1650775, @aaron.ballman wrote:

In D66919#1650174, @dexonsmith wrote:

This could cause a lot of churn in existing projects (especially with static void foo()), without giving Clang any new information. I'm wary of this.

Those projects likely aren't aware they're using prototypeless functions, which are trivial to call incorrectly. I suspect this diagnostic will find real bugs in code.

To be clear, my understanding is that -Wstrict-prototypes already warns on non-prototype declarations like this:

void foo();

we just don't warn on non-prototype defining declarations, where the meaning is unambiguous:

void foo() {}

In D66856#1649721, @erik.pilkington wrote:

In D66856#1648809, @aaron.ballman wrote:

I'm wondering whether this should even warn pedantically. There are no format specifiers that apply directly to a _Bool datatype, so the user is left making a choice as to what specifier fits best. I think hhd and hhu are both equally reasonable types, as are the d and u groups directly -- I don't think we should warn on either of those specifiers with a _Bool argument. I could maybe see a case for pedantically diagnosing h, l, and ll prefixes with _Bool because that seems a bit type-confused to me. c as a specifier seems weird (given that false values will potentially terminate the string suddenly depending on terminal behavior, IIRC), but lc seems like type confusion again.

Hmm... on second though I think the l and ll prefixes are worth -Wformat proper, since that can result in printing a half-initialized integer when the argument ends up on the stack (we generate movl $0,(%rsp) for the first stack argument).

In D66919#1650174, @dexonsmith wrote:

This could cause a lot of churn in existing projects (especially with static void foo()), without giving Clang any new information. I'm wary of this.

In D66850#1649708, @piscisaureus wrote:

In D66850#1648776, @aaron.ballman wrote:

LGTM, but missing a test case.

In D66850#1648557, @sidorovd wrote:

LGTM. I'm not an expert in JSON, but may be it makes sense to move the change a line earlier before creation of pointer representation?

I would prefer it remains where it is -- having the 0x0 in the output for a null pointer is a good thing because it conveys more information than a totally empty Type object. We're accidentally inconsistent about this currently (Decl prints 0x0 but Stmt gives an empty object).

I don't disagree, but I would argue that "0x0" is a rather poor choice, since now the consumer has to treat the "id" field as an opaque value, except when it's "0x0". A better choice would be to use JavaScript null to represent null pointers.

Aside from the few remaining nits, I think this is almost ready to go.

In D66397#1649353, @xbolva00 wrote:

In D66397#1647455, @aaron.ballman wrote:

Adding @rsmith to see if we can make forward progress on this patch again.

On the other side, I don't want to waste Richard's time since I dont want to extend (variables and macros are controversal topic anyway) it more now. I promised to @jfb to handle (2 ^ 64) - 1 as follow up patch and the promised patch is here..

I'm wondering whether this should even warn pedantically. There are no format specifiers that apply directly to a _Bool datatype, so the user is left making a choice as to what specifier fits best. I think hhd and hhu are both equally reasonable types, as are the d and u groups directly -- I don't think we should warn on either of those specifiers with a _Bool argument. I could maybe see a case for pedantically diagnosing h, l, and ll prefixes with _Bool because that seems a bit type-confused to me. c as a specifier seems weird (given that false values will potentially terminate the string suddenly depending on terminal behavior, IIRC), but lc seems like type confusion again.

LGTM, but missing a test case.

In D64671#1644280, @jpakkane wrote:

I used stdint to replicate a real world use case as I'd imagine those types would match this search quite heavily.

Adding @rsmith to see if we can make forward progress on this patch again.

In D62571#1646227, @domdom wrote:

Rebased onto master, clang format the patch.

Merge conflict resolve by having the bitcast of the field reference happening after recording access index.

I'm not an LLVM person, but I just wanted to double-check: is this correct even in the face of situations where malloc() and friends return a non-null pointer that is *not* dereferenceable? e.g., when someone calls malloc(0) with certain libc implementations?