- User Since
- Jan 10 2013, 2:43 PM (275 w, 4 d)
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:
Fri, Apr 20
Thu, Apr 19
Makes sense to me.
Tue, Apr 17
+1 to just adding a dedicated Wno flag for the new warning instead of
coming up with this new spelling.
Thu, Apr 12
We think there's an UB check for this already, but v8 isn't UB clean.
Wed, Apr 11
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?)
Tue, Apr 10
$ svn merge -c -329714 .
- Reverse-merging r329714 into '.':
- Recording mergeinfo for reverse merge of r329714 into '.': U .
$ svn merge -c -329693 .
- Reverse-merging r329693 into '.':
- Recording mergeinfo for reverse merge of r329693 into '.': U .
$ svn merge -c -329684 .
- Reverse-merging r329684 into '.':
- 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.'
Transmitting file data ...................done
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?
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.
Fri, Apr 6
Wed, Apr 4
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:
kcc, if samsonov no longer does llvm reviews, who can review this?
Tue, Apr 3
Mon, Apr 2
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.)
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
Sun, Apr 1
Fri, Mar 30
Mar 19 2018
Awesome, thanks! One nit below:
Mar 12 2018
Maybe we should also print something like
Mar 11 2018
Mar 10 2018
Mar 9 2018
Mar 8 2018
Since this affects analysis-based warnings, have you checked if this patch has any effect on compile times?
Mar 7 2018
Fix LGTM, one optional comment below.
Mar 2 2018
Feb 28 2018
Feb 15 2018
So should we revert https://reviews.llvm.org/D42632 for now?
Did this land?
Feb 13 2018
Feb 12 2018
some attr merging
Feb 9 2018
Feb 8 2018
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.
Feb 7 2018
Err sorry, landed in rL310382.
What's the status here?
Feb 6 2018
I went ahead and landed this with the comments requested by Duncan in r324385. (http://llvm.org/viewvc/llvm-project?view=revision&revision=324385)
Feb 5 2018
This should be in clang-tools-extra next to clang-tidy, clang-include-fixer, clangd etc, not in the main compiler repo, right?