- User Since
- Mar 18 2014, 10:33 AM (174 w, 2 d)
Tue, Jul 18
Sun, Jul 16
Thu, Jul 13
Wed, Jul 12
This LGTM. Mehdi, do you have any other concerns?
Sun, Jul 9
Assuming nothing in arena<>...Db actually changed (just moved), this LGTM if you split the move into a separate prep commit.
Thu, Jul 6
Ping! Hal, you committed r283051. Do you have a problem with this?
Mon, Jun 26
Fri, Jun 23
I followed the other availability macros in using "strict", but I'm not sure this was the right decision. The documentation seems to recommend not using "strict" (it says that weakly-linking is almost always a better API choice).
libc++ is one of the exceptions. Use strict.
Thu, Jun 22
Wed, Jun 21
Right now, sys::getProcessTriple returns the LLVM_HOST_TRIPLE, whose system version might not be the actual version of the system on which the compiler running.
Jun 19 2017
We use the -Wc++NN-compat- prefix on all the other subwarnings of -Wc++NN-compat warnings
I think -Wc++1z-mangling is best. IMO, with C++1z in the name, it's already clear that this is about compatibility. I also don't think we need to get as specific as -Wc++1z-compat-exception-spec.
Jun 18 2017
Fixed in r305661.
Committed in r305647.
Jun 17 2017
Jun 15 2017
This is the right idea, although it only covers macOS.
Jun 14 2017
Jun 13 2017
You mention that the lock-free deque gives a speedup. I agree that should be left for a follow-up, but what kind of speedup did it give?
Jun 1 2017
May 29 2017
The current patch seems to be missing context on Phab (-U999999). Please remember to add it on the next iteration.
May 25 2017
I'm probably a good person to review this, but it's been a while since I've thought this so I might need help to refresh my context. This use-tracking code is unfortunately rather subtle.
Actually, looking through the comments, it appears that everyone (eventually) agreed with the approach in the patch. I agree too. LGTM.
May 23 2017
May 22 2017
May 21 2017
May 19 2017
May 17 2017
May 16 2017
The testcase updates look good to me, and demonstrate the patch is doing what's intended. It might be worth adding a couple of tests for multiple branch targets (switch statements), since I'm not convinced everything adds up to 1 right now.
May 14 2017
May 13 2017
May 9 2017
May 3 2017
Breaking the ABI of that configuration SGTM.
Thanks for being flexible; committed in r302095. Maybe longer term we could add a CMake configuration for whether to include the tests, but naming it might be hard ;).
May 2 2017
(I must have misclicked somewhere.)
This LGTM too.
Apr 29 2017
This LGTM, and it's liable to bitrot if it hangs out here any longer. We can always iterate in tree if we find a better way to organize the markup and/or tests.
Apr 28 2017
Upgrade LGTM. (I'm not planning to review the rest, but let me know if you need me to take a closer look.)
Apr 27 2017
rsmith: I just noticed you still have a red "X" here (since Phab won't let me "close"). I think I addressed your comments, but let me know if you want me to revert until you have time to look.
Thanks for the reviews! Committed in r301593.
Fixed in r301592.
(Both changes SGTM, but I doubt you're looking for my advice on the CMake logic.)
Apr 26 2017
Oh, and it would be nice to split out Preprocessor::diagnoseMissingHeaderInUmbrellaDir in a separate NFC commit ahead of time.
LGTM after a couple of changes inline.
Apr 22 2017
Apr 21 2017
Thanks Eli and Richard.
Committed in r301060.
Since I haven't heard from Marshall and Hal's fine with the less-future-proof std::is_floating_point, I'll commit that and we can iterate in tree. Just running tests.
Thanks for the review, Eli.
Apr 20 2017
It looks like __INTPTR_TYPE__ was introduced in r64495 in order to help define intptr_t, and then they diverged in r89237.
Here's an updated patch that uses __SIZE_MAX__ and also handles the other pointer-like integers.
Hmm... presumably, this test should pass:
Marshall, are you fine with figuring this out in-tree? Or should I revert to using std::is_floating_point for now?
Thanks for the review. __SIZE_MAX__ makes sense to me; and better even if not for the #if requirement. I'll do that for both SIZE_MAX and PTRDIFF_MAX when I get a chance.
Apr 19 2017
This was committed in some form a long time ago.
Eric: somehow I never got an email notification for your review :/. If Marshall is okay with the current state, I'll document that before commit.
Marshall, that's what I assumed originally, but I figured Hal had some non-standard-but-worth-supporting use case in mind.