- User Since
- Aug 19 2016, 10:21 AM (92 w, 1 d)
Fri, May 25
Wed, May 23
See also @pcc's proposal: http://lists.llvm.org/pipermail/llvm-dev/2018-May/123514.html
Tue, May 22
Colocate CHECK lines with declarations
Fri, May 18
Why do we hardcode compiler IDs/versions here instead of just checking whether the current compiler supports the flag?
Tue, May 15
Sun, May 13
Fri, May 11
Emailed cfe-dev: http://lists.llvm.org/pipermail/cfe-dev/2018-May/057934.html
Wed, May 9
If you're not planning to move forward with the diff, it's better to abandon it rather than close it. Close usually implies it was committed.
Yeah, I think we all agree now that a portability warning isn't really tractable. Note that even for the warnings that motivated this diff, they should have only fired if size_t and NSInteger had separate types, so it wasn't a portability warning in that sense to begin with (as in, it would only warn if there was a mismatch for your current target, not if there was a potential mismatch for any target).
Tue, May 8
Generating the patch from libc++ is fine (and this patch looks like it has sane paths).
Mon, May 7
Note that the alignment matters in addition to the size.
Is this fixing the same issue as D46214?
Sat, May 5
No worries; thanks for all the other reviews, and for adding Michael as a reviewer to this one :)
Fri, May 4
Thu, May 3
Wed, May 2
Tue, May 1
Mon, Apr 30
Makes sense to me. You may wanna wait for @zturner, but I can't really imagine any benefit to restricting this to MSVC.
Apr 26 2018
Update with @grimar's suggestion
Apr 25 2018
(Gentle ping. This isn't urgent, but it would be nice to have.)
Only read CMAKE_EXE_LINKER_FLAGS, to be consistent with CMake under CMP0056
Apr 24 2018
Apr 20 2018
Apr 19 2018
LGTM. I'd tried to clean all stale _MSC_VER conditionals in the past, but I must have missed some.
Typo in the summary: refernece -> reference
Apr 18 2018
LGTM; it's an obvious NFC patch.
Apr 12 2018
Eric accepted on IRC.
Apr 11 2018
This LGTM given that Eric accepted your previous change. I'm happy to accept it in a day or two if he hasn't gotten the chance to take a look by then.
Apr 9 2018
Apr 6 2018
Patch is missing context.
Apr 4 2018
Apr 3 2018
This doesn't pass all tests right now, and I'll also need to change the test in accordance with the review comments.
Finally got a chance to dig into some of the failures this patch is causing. Here's an example crasher (run with clang -cc1 -triple i686-windows -emit-llvm):
Mar 27 2018
Mar 26 2018
Mar 22 2018
PURE_WINDOWS is all Windows except Cygwin. It's an LLVM thing, not a CMake thing.
Mar 20 2018
Thanks Reid. I still need to look into why this is causing some existing tests to crash, but I'll also adjust the test.
Seems fine to me, since windows-itanium is unaffected.
Mar 19 2018
I would imagine there was a reason this configuration was unsupported on macOS – perhaps something to do with libc++/libc++abi being the default system libraries on that platform?
Sweet, thanks for the update.
@DHowett-MSFT the reviewers look fine to me. Reid is the code owner for clang's MSVC compat support. David doesn't work on this stuff directly anymore, I think, but he's still pretty active in code reviews for it. I'm adding Saleem, who's also pretty active on Windows stuff.
LGTM with the comments addressed.
Could you upload the patch with context (-U9999) please?