smaller diff

What's the next step here?

Thanks for taking a look!

In D67461#1667296, @hans wrote:

Nice!

But is there no help text or user manual that needs to be updated?

Hopefully this is rare enough that putting it under an existing warning flag won't be too inconvenient.

Do you think this should land with the comment FIXME for now? It improves formatting of this language feature when that heuristic is not used, and changing the heuristic is independent of the rest of this patch.

The test added in this change has a file leak that causes all possible temp file patterns to be created, at which point it starts failing, see https://bugs.llvm.org/show_bug.cgi?id=43278 (Diego, I tried to cc you, but bugzilla didn't know your email address.) Can you please take a look, and revert if it takes a while to investigate? (remove-funcs seems to have the same problem.)

Did you collect metrics on number of true/false positives of this warning?

Another question about this, sorry. Do you know _why_ C++20 is more restrictive than C99 wrt "mixture of designated and non-designated initializers in the same initializer list is a C99 extension"? Is there some interaction with other C++ features that makes the C99 behavior difficult in C++20?

fix nits

Thanks for the thorough review! Indeed, this still gets []<bool b = true && false>(A &&a){}(); wrong, for the reason you mention.

I don't know what the long-term plans for this script are. I don't have a strong opinion, but weakly agree that it'd be good if it went away. In that case, I personally wouldn't add a warning and just make it temporarily do the (usually :P) right thing of using origin/master as upstream if there's no explicit upstream. If there's a warning however, it'd be nice if it included git checkout -b myNewBranch origin/master as suggested command to run, since I found that difficult to find. (Thanks for mentioning it!)

We have the old TODO of changing this warning to be emitted at enum use time (e.g. when Foo_Val is compared to 0) instead of declaration time. Maybe that's a better fix. Or is implementing that very involved?

PS – Hi @thakis. I think it is unfair and incorrect to say that "nobody noticed" that the existing behavior was problematic. I noticed. And I'd wager others did also. That being said, the kinds of people that would notice that "origin" is hardcoded into git-llvm also tend to be the kinds of people that can workaround this git-llvm bug until they get tired of doing so and submit a patch.

To add some more words, previously we could git checkout -b foo, hack a bit commit, and run this script. Now this requires additional, long flags. I don't think making a corner case work that nobody noticed being broken for many months is worth making everything more inconvenient for everyone.

Similar to leonardchan, this broke my workflow too. Please don't break user's workflows. If there's no good fix for this, I think this should be reverted.

delcypher, kubamracek: Would you prefer if someone else looked at this?

Re llvm-undname: That seems to match behavior of MSVC's undname, so I'd say that's all good :)

ps: Thanks for the patch, this is great :)

Does this pass ninja check-clang? If so, can you add a test for the new mangling, based on the manglings in the godbolt example? See clang/test/CodeGenCXX/mangle-ms-* for many examples; a new file mangle-ms-cxx20.cpp might be appropriate here. You can run bin/llvm-lit -vv clang/test/CodeGenCXX/foo.cpp to run a single lit test for iterating on the local test.

Looks horrible, please commit :)

For this to fire and be a true positive, you'd have to have an incorrect declaration but a correct definition of a function. I can't remember ever running into this – and the compiler seems to be in a better place to print a diagnostic for this case in almost all cases (except for if the TU with the correct definition doesn't happen to see the incorrect declaration).

The file that provides the definition may have a mismatch of the declaration and the definition, e.g.

This is a cool idea, but I'm struggling to come up with a scenario where it's actually useful :)

This is a cool warning. Could the note have a fixit attached to it, so that it's easy to mass-clean-up the warning automatically?

delcypher, kubamracek : ping.

Why expand to int3 instead of nop?

There are enough highly capable reviewers actively looking at this; resigning from this one :)

lgtm, with a suggestion for an (I think?) missing test:

In D66824#1651630, @mstorsjo wrote:

In D66824#1651587, @thakis wrote:

I'd vote for not running complicated code unless we have to, so I'd keep this MinGW only.

TBH it's not all that complicated, but in any case, none of that code should run in the cases that are currently supported anyway. It's just choosing between if you want to keep the error, to keep matching link.exe, or be more tolerant and support multiple resource objects even for non-MinGW setups.

Making the tests in this patch pass sounds good to me too :)

lgtm, nice.

Nice!

I'd vote for not running complicated code unless we have to, so I'd keep this MinGW only.

Yes, that makes sense, thanks.

We're getting a bunch of errors looking like ../../AppsListViewController.m:11:37: error: incompatible block pointer types assigning to 'void (^)(__strong id<AppCellProtocol>)' from 'void (^)(AppCollectionViewCell *__strong)' on code that looks fairly harmless to me. It looks something like this: