In any case, when you see a test failing on bots and the fix isn't obvious,
revert first to get the bots back green.

Sorry about the belated review comment, but:

r330455

r330427, thanks!

Makes sense to me.

+1 to just adding a dedicated Wno flag for the new warning instead of
coming up with this new spelling.

We think there's an UB check for this already, but v8 isn't UB clean.

This broke some large integration test on arm64 in Chromium (https://crbug.com/831145 – doesn't yet have any information other than "something is broken" and that we bisected it to this revision). It sounds like the patch got rewritten during review; does "formally allowed by the standard, but extremely likely to break things and surprise people" still apply? It sounds like this is supposed to be behavior-preserving for values that aren't NaN and Inf, but it does change behavior for those two? (If so, in what way?)

$ svn merge -c -329714 .

• Reverse-merging r329714 into '.':

A test/Frontend/ftime-report-template-decl.cpp

• Recording mergeinfo for reverse merge of r329714 into '.': U .

$ svn merge -c -329693 .

• Reverse-merging r329693 into '.':

U test/Frontend/ftime-report-template-decl.cpp

• Recording mergeinfo for reverse merge of r329693 into '.': U .

$ svn merge -c -329684 .

• Reverse-merging r329684 into '.':

D test/Frontend/ftime-report-template-decl.cpp
U include/clang/Frontend/FrontendAction.h
U lib/Parse/ParseTemplate.cpp
U lib/Frontend/FrontendAction.cpp
U lib/Lex/Pragma.cpp
U lib/CodeGen/CodeGenAction.cpp
U include/clang/Parse/Parser.h
U include/clang/Sema/Sema.h
U include/clang/Lex/HeaderSearch.h
U lib/Parse/Parser.cpp
U lib/Frontend/ASTMerge.cpp
U lib/Frontend/FrontendActions.cpp
U lib/Frontend/CompilerInstance.cpp
U lib/Sema/SemaChecking.cpp
U lib/Sema/SemaExpr.cpp
U lib/Sema/Sema.cpp
U lib/Sema/SemaDecl.cpp
U lib/Sema/SemaTemplate.cpp
U lib/Lex/PPMacroExpansion.cpp
U lib/Lex/HeaderSearch.cpp

• Recording mergeinfo for reverse merge of r329684 into '.': U .

$ svn commit -m 'Revert r329684 (and follow-ups 329693, 329714). See discussion on https://reviews.llvm.org/D43578.'
Sending include/clang/Frontend/FrontendAction.h
Sending include/clang/Lex/HeaderSearch.h
Sending include/clang/Parse/Parser.h
Sending include/clang/Sema/Sema.h
Sending lib/CodeGen/CodeGenAction.cpp
Sending lib/Frontend/ASTMerge.cpp
Sending lib/Frontend/CompilerInstance.cpp
Sending lib/Frontend/FrontendAction.cpp
Sending lib/Frontend/FrontendActions.cpp
Sending lib/Lex/HeaderSearch.cpp
Sending lib/Lex/PPMacroExpansion.cpp
Sending lib/Lex/Pragma.cpp
Sending lib/Parse/ParseTemplate.cpp
Sending lib/Parse/Parser.cpp
Sending lib/Sema/Sema.cpp
Sending lib/Sema/SemaChecking.cpp
Sending lib/Sema/SemaDecl.cpp
Sending lib/Sema/SemaExpr.cpp
Sending lib/Sema/SemaTemplate.cpp
Transmitting file data ...................done
Committing transaction...
Committed revision 329739.

"False positive" means "warning fires but didn't find anything
interesting", not "warning fires while being technically correct". So all
these instances do count as false positives.

This landing made our clang trunk bots do an evaluation of this warning :-P It fired 8 times, all false positives, and all from unit tests testing that operator= works for self-assignment. (https://chromium-review.googlesource.com/c/chromium/src/+/1000856 has the exact details) It looks like the same issue exists in LLVM itself too, https://reviews.llvm.org/D45082

Sorry, that comment was meant for the commit that tweaked the warning in clang. I'll repost there.

This landing made our clang trunk bots do an evaluation of this warning :-P It fired 8 times, all false positives, and all from unit tests testing that operator= works for self-assignment. (https://chromium-review.googlesource.com/c/chromium/src/+/1000856 has the exact details)

Also, please add cfe-commits to clang changes. Since this wasn't here and the patch wasn't seen by clang folks, since ftime-report-template-decl.cppwarnings is still failing on the bots (e.g. http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/16388) and since it breaks the shared bot, maybe we should revert for now and reland when the issues are addressed?

r329698, thanks!

Thanks for the pointer! I commented on https://reviews.llvm.org/D43578; I'm hoping there's a fix that doesn't require making so many libs depend on clang's IR library.

@davezarzycki remarks in https://reviews.llvm.org/D45485 that this breaks the shared build. The proposed fix there is to make several of clang's modules depend on LLVM's IR library ("Core"). This seems weird to me for two reasons, one architectural, one form a build point of view:

Can you link to the recent timer changes?

Unless someone shouts, I'm going to submit this CL in the next few days. If someone does depend on this code, we can easily put it back.

ddunbar: ping

lgtm, but I don't know this code super well, so wait for ruiu too.

It's not just MSVC, see the polly bot.

This breaks compile on various bots:
http://lab.llvm.org:8011/builders/sanitizer-windows/builds/26736/steps/build%20clang%20lld/logs/stdio
http://lab.llvm.org:8011/builders/lldb-x86-windows-msvc2015/builds/21016/steps/build/logs/stdio
http://lab.llvm.org:8011/builders/perf-x86_64-penryn-O3-polly-parallel-fast/builds/12248/steps/build%20stage%201/logs/stdio
(and others)

bruno: ping

kcc, if samsonov no longer does llvm reviews, who can review this?

I’d expect _LIBCPP_FORCE_NODISCARD to win over _LIBCPP_DISABLE_NODISCARD_AFTER_CXX17, but /shrug. Thanks!

(See also https://bugs.llvm.org/show_bug.cgi?id=10011 for a somewhat related discussion.)

In D44883#1048751, @dblaikie wrote:

Historically Clang's policy on warnings was, I think, much more
conservative than it seems to be today. There was a strong desire not to
implement off-by-default warnings, and to have warnings with an
exceptionally low false-positive rate - maybe the user-defined operator
detection was either assumed to, or demonstrated to, have a sufficiently
high false positive rate to not meet that high bar.

Actually, it doesn't pass on non-Windows either: http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/27665/steps/test/logs/stdio

The test added here doesn't pass on Windows, and the change breaks another test on Windows: http://lab.llvm.org:8011/builders/clang-x64-ninja-win7/builds/9794

Reverted in r328978 to green up the bots.

Looks like this made tablegen crash: http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-buildserver/builds/21791/steps/ninja%20build%20local/logs/stdio

r328970, thanks!

r328879

r328877, thanks!

Awesome, thanks! One nit below:

r327261, thanks!

r327262, thanks!

Maybe we should also print something like

Thanks!

r327124, thanks!

Since this affects analysis-based warnings, have you checked if this patch has any effect on compile times?

r326960, thanks!

rnk comment

sort unstably

Good luck!

Fix LGTM, one optional comment below.

In D43978#1025330, @thakis wrote:

In D43978#1024821, @zturner wrote:

Apparently this *still* isn't quite right. The scenario presented to me was this:

1. User does a clean build, hash X gets written into PDB, hash Y gets written into EXE.
2. User archives EXE and PDB on symbol store. EXE is in some directory X, PDB is in some directory Y.
3. User adds some blank lines and rebuilds. EXE doesn't change but PDB does.
4. User re-archives. EXE is unchanged so doesn't get updated on the symbol store, but PDB does. PDB now gets archived in directory Z.
   
   So now there is only one EXE, Y, but there are two PDBs, X and Z. If you have the new source code, you will run the EXE, it will find it under directory Y in the sym store, map to the old PDB X, and the source won't match.

I don't understand this scenario. If the pdb changes, doesn't the RSDS header in the exe linking to the binary have to change as well?

In D43978#1024821, @zturner wrote:

Apparently this *still* isn't quite right. The scenario presented to me was this:

1. User does a clean build, hash X gets written into PDB, hash Y gets written into EXE.
2. User archives EXE and PDB on symbol store. EXE is in some directory X, PDB is in some directory Y.
3. User adds some blank lines and rebuilds. EXE doesn't change but PDB does.
4. User re-archives. EXE is unchanged so doesn't get updated on the symbol store, but PDB does. PDB now gets archived in directory Z.
   
   So now there is only one EXE, Y, but there are two PDBs, X and Z. If you have the new source code, you will run the EXE, it will find it under directory Y in the sym store, map to the old PDB X, and the source won't match.

r326357, thanks!

So should we revert https://reviews.llvm.org/D42632 for now?

Did this land?

r325052, thanks!

some attr merging

Nice!

I was about to say "please make sure this honors -fdebug-prefix-map" but I suppose codeview probably contains so many absolute paths at the moment that we should instead have a concerted effort to make debug info cwd-independent at some later point instead.

Err sorry, landed in rL310382.

What's the status here?

I went ahead and landed this with the comments requested by Duncan in r324385. (http://llvm.org/viewvc/llvm-project?view=revision&revision=324385)

This should be in clang-tools-extra next to clang-tidy, clang-include-fixer, clangd etc, not in the main compiler repo, right?