- User Since
- Mar 18 2014, 10:33 AM (183 w, 5 d)
Tue, Aug 29
Yes, abandoning seems reasonable. I think the main idea of step 4 when I filed PR22780 was to reduce memory usage, but this didn't turn out to be a bottleneck.
Aug 25 2017
Aug 23 2017
Aug 21 2017
LGTM! Thanks for reworking this.
Aug 18 2017
Aug 17 2017
Who changed the behaviour? Can we get them to review the behaviour of the upgrade?
Aug 14 2017
Aug 9 2017
I have a few more nitpicks; LGTM after you fix those.
Aug 8 2017
Aug 7 2017
Oh, also found a couple of things you should likely split into prep commits to simplify this patch.
This looks like a great improvement. I've littered the patch with nit-picks, but my main concern is that there aren't any unit tests for the new data structures. I wonder if there's a hacky way to set that up...
Jul 28 2017
Jul 27 2017
The code changes and test/Bitcode/upgrade-dbg-value.ll LGTM.
Should there also be an update to LangRef?
Jul 18 2017
Jul 16 2017
Jul 13 2017
Jul 12 2017
This LGTM. Mehdi, do you have any other concerns?
Jul 9 2017
Assuming nothing in arena<>...Db actually changed (just moved), this LGTM if you split the move into a separate prep commit.
Jul 6 2017
Ping! Hal, you committed r283051. Do you have a problem with this?
Jun 26 2017
Jun 23 2017
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.
Jun 22 2017
Jun 21 2017
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.