- User Since
- Mar 18 2014, 10:33 AM (213 w, 6 d)
Another high-level note (besides adding tests): please resubmit the patch with full context (e.g., git diff -U9999999 HEAD^..).
That doesn’t seem like the right place for the newline; instead, it should be wherever the asterisks were printed.
Sun, Apr 22
I don't think this is quite right. I know at least make-based incremental builds wouldn't deal well with this.
Thanks for working on this! I've just done a pass through for style, and I have a few nits to pick.
Mon, Apr 16
Fri, Apr 13
Hal requested splitting the patch in two, since the two features are separable, but they both still seem to be here. Perhaps start with the prefix patch?
Wed, Apr 11
I'd prefer to have this be as close as possible to the libcxxabi demangler (and use the same data structures) so that it's easy to detect when they diverge. If we ever move to monorepo hopefully there will be a way to factor them out into a single implementation.
Mon, Apr 9
Have you confirmed this doesn't need to depend on the deployment target? I.e., do we need the old logic sometimes when back-deploying?
Mon, Apr 2
Tue, Mar 27
Mar 22 2018
I agree with Saleem and Bob: __is_target_* is not confusing here and seems to be a straightforward spelling. It has also already shipped in LLVM 6.0.0: it would be awkward to stop supporting this syntax.
Mar 8 2018
Feb 16 2018
Feb 4 2018
Jan 31 2018
If it's possible to separate the bugfix into a separate commit (either before or after), I think you should.
Jan 29 2018
LGTM once you clean up the #if 0.
Jan 22 2018
Thanks for working on this!
Jan 11 2018
Jan 10 2018
Jan 4 2018
Jan 3 2018
Dec 13 2017
Dec 10 2017
Akira and/or John should take a look instead of me.
Dec 9 2017
Dec 8 2017
This seems correct to me. I wouldn't mind having someone else back me up though.
Committed in r320208.
Dec 7 2017
LGTM, after you try to shrink the testcase. Thanks for tracking this down!
Dec 6 2017
Nov 6 2017
Nov 2 2017
Nov 1 2017
I have a few nitpicks, but otherwise this LGTM. I'd like to wait for someone on the CUDA side to confirm though.
Oct 26 2017
Oct 25 2017
Oct 24 2017
std::sort and array_pod_sort both use quicksort
Oct 10 2017
Aug 29 2017
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