- User Since
- Jan 10 2013, 2:43 PM (287 w, 3 d)
Fri, Jul 13
Thanks! Landed with a test in r337084.
Thu, Jul 12
Looks good to me, thanks!
Wed, Jul 11
Thanks for taking a look!
lgtm assuming you ran this on some large code base to make sure it doesn't have false positives.
Fri, Jul 6
Thanks for the patch! I wonder if this should be behind some flag (/makePDBPathsAbsolute or something) and not be done by default. If we have a flag, the flag could also optionally provide a different prefix than pwd (/makePDBPathsAbsolute:c:\src), which might be useful for cross builds. I'd like to hear ruiu's thoughts on this.
Thu, Jul 5
Mon, Jul 2
Thanks for working on this! I'd be happy to see this fixed. It also likely helps with PR36676.
Thanks, landed with nit addressed in r336097.
Fri, Jun 29
For your convenience: https://godbolt.org/g/KXxbKb
Wed, Jun 27
The things I pointed at only do case-insensitivity. I haven't seen anything using backslashes in the builds I worked on. (Is that for -I flags?) I'd think that the backslash bits can probably be implemented with less overhead.
There are 2 other patches out there for the case sensitivity. Neither landed, because the performance hit form this approach is pretty big, and it's not necessary: You can either put the Win SDK into a ciopfs mount (example: https://cs.chromium.org/chromium/src/build/vs_toolchain.py?type=cs&q=ciopfs&sq=package:chromium&g=0&l=377 , used by chrome's win/cross build https://chromium.googlesource.com/chromium/src/+/master/docs/win_cross.md) or use -ivfsoverlay (example: http://llvm-cs.pcc.me.uk/cmake/platforms/WinMsvc.cmake#104 , used by llvm's win/cross build).
Tue, Jun 26
Tried moving it up in r335608.
Mon, Jun 25
Still looks good, ship it! One more suggestion about additional test coverage (but maybe it's already there and I'm just missing it).
Fri, Jun 22
This code is live when reading pchs, correct? Does this have any measurable perf impact on deserializing pchs for, say, Cocoa.h or Windows.h?
Thu, Jun 21
PCHs aren't compatible with themselves if only the compiler revision changes, so I'm not sure changing that field should be worse than a regular compiler revision update (which happens at every commit). But I don't know what this field is for. I don't remember any trouble from me changing it though, and if it was bad to change it I'd hope there'd be a comment right above the field telling us why. (If I had run into problems, I would've expected me to add a comment like this.)
Looks like pretty straightforward plumbing. I can't think of a better place than ASTContext either. I think this is fine with your two XXXs just removed.
Thanks for explaining, makes sense to me.
Tue, Jun 19
Mon, Jun 18
Thanks! I'd keep it DefaultIgnored: I'd be annoyed if I'd get these by default in C++98 mode.
Jun 15 2018
Jun 12 2018
What's the status here? The MS STL has this feature under _HAS_NODISCARD and it's super useful.
Jun 11 2018
(FWIW since this was enabled again we had at least two bugs due to this -- https://crbug.com/845816 https://crbug.com/851415 -- and one confused thread at https://groups.google.com/a/chromium.org/forum/#!topic/cxx/W5wma_HXWOo )
Jun 8 2018
ruiu: This review has now gone on for a week, with one cycle per day due to timezones. Since the comments are fairly minor nits, do you think you could address them yourself while landing this, in the interest of not spending another week on this?
Jun 4 2018
support > 8k long commands on win
Jun 1 2018
May 31 2018
May 30 2018
May 29 2018
But GetExecutablePath isn't called if -no-canonical-prefixes is passed, is
it? (See the first link I pasted above)
I did test this locally before sending it out, and the -resource-dir arg passed to cc1 is relative with this patch:
May 21 2018
This was reverted by 332742. Sorry, I failed at syncing up my checkout.
Did you land the revert? I'm seeing the same failure running tests locally as on http://lab.llvm.org:8011/builders/clang-cmake-x86_64-avx2-linux/builds/4445/steps/ninja%20check%201/logs/FAIL%3A%20LLVM%3A%3APR37310.mir
Reverted in r332838. Please don't keep trunk broken for days.
Sorry about the breakage, I wasn't aware of that test failure and never got any email (probably because the bot has been red before). I'll try to fix make_itanium_abi_triple and test it on my Windows box tomorrow before resubmitting.
May 20 2018
Reverted in r332822 / r332823.
…hm looks like https://reviews.llvm.org/rL332750 touched files in both clang and llvm, which is probably why the diff looks super confusing on phab. I'll revert it in two pieces, I don't know how to commit changes to llvm and clang in one revision.
This changes the default triple on Windows from x86_64-pc-win32 to x86_64-pc-windows-msvc. This breaks at least lit's make_itanium_abi_triple() (http://llvm-cs.pcc.me.uk/utils/lit/lit/llvm/config.py#234)
(sorry, hit cmd-return accidentally, I will come in again…)
This changes the default triple on Windows from x86_64-pc-win32 to x86_64-pc-windows-msvc. This breaks at least
May 18 2018
Various chromium test cases started failing on the first build with this change, and I don't see anything suspicious chromium-side: https://ci.chromium.org/buildbot/chromium.clang/CFI%20Linux%20%28icall%29/9886 Any chance that could be due to this?
It has the same effect and doesn't require yet another compiler call at cmake time, and once we uprev gcc requirements to 4.9 (soooon) we can just delete the whole condition in the parens.
May 17 2018
Please do, tzik doesn't have commit access yet.
The test seems to fail on some bots: http://lab.llvm.org:8011/builders/clang-with-thin-lto-ubuntu/builds/10372
May 16 2018
Aha, they used to be different before https://reviews.llvm.org/rL201316
How about I use LTDL_SHLIB_EXT instead of LLVM_PLUGIN_EXT in the cmake files? (I'm not sure why we have both, they're set to the same thing: http://llvm-cs.pcc.me.uk/cmake/modules/HandleLLVMOptions.cmake#132)
Hm, does this only happen on the ps4 bots? Maybe that toolchain sets some default behavior that changes if this flag is exposed or something. Does it repro if you pass -triple x86_64-scei-ps4-ubuntu in the RUN line?
May 15 2018
My guess is it's because the clang tests check if the edit distance is > 1, when you really want to check > 0 instead. Maybe you can try changing that in clang, then relanding?
(accidentally hit cmd-return instead of return; i'll come in again)
As far as I know ToolingCore gets linked into all kinds of binaries (e.g. clang) – is this really the best place for this code?
May 14 2018
lgtm, but please credit where at least parts of this are from :-)