- User Since
- Mar 18 2014, 10:33 AM (225 w, 5 d)
Wed, Jul 11
Tue, Jul 10
Thanks! (I'm not comfortable signing off though, since I don't know much about dllimport.)
Is it impossible to also test this locally (in the LLVM repo)? It will be hard to hack on this file if there aren't local tests.
Mon, Jul 9
Sat, Jul 7
Thu, Jul 5
I've started an RFC on llvm-dev to discuss whether we should make this change:
Wed, Jul 4
Awesome. I wonder, would it be worth measuring compile-time impact for optimized builds?
Tue, Jul 3
FWIW, this change LGTM... but I wouldn't mind hearing from Eric or Marshall before commit.
Mon, Jul 2
What was the numbering before?
Fri, Jun 29
Thu, Jun 28
I'm not sure this technique will work in the new pass manager, since analyses are run on-demand IIRC. I wonder if it would be better to leave these as passes, and implement -print-after-all by inserting -print-module/etc. passes into the pipeline as the pipeline is being constructed. You could avoid adding extra -print-module statements in cases where that pass already exists.
This seems like a reasonable thing to do. Please add tests to check the output of both pass managers.
Wed, Jun 27
What's the effective change on the command-line? Does this map to -nostdinc? To -nostdinc++?
Please close this and open a new one. Adding cfe-commits after the fact will fail to send the patch/description to cfe-commits, and not everyone is on llvm-commits.
Resigning as reviewer (thanks for fixing the complexity); I'll let the other reviewers sort out if SmallVector is an even better solution here.
Tue, Jun 26
Mon, Jun 25
Okay, this LGTM then.
What's the potential overhead of this choice (memory, runtime)?
Sat, Jun 23
Updating the patch since I decided to just lift set_size() up to SmallVectorBase. My ASan-ified check-llvm and check-clang came back happy.
Committed in r335421.
Fri, Jun 22
I still think you might be able to use an Optional (see my inline comment) but I don't need to see this again.
Thu, Jun 21
Wed, Jun 20
I just have a couple of additional (minor) comments on the code, although I admit it was hard to be thorough as a it's a monolithic patch. I might have preferred if this were committed incrementally, adding support for 1-2 constructs at a time. Because the lexing/parsing code is fairly mechanical, I think splitting it up now would be quite likely to introduce errors so I'm not going to ask for that.
Tue, Jun 19
I've just had time to skim a few files; feel free to ping me privately if I don't continue the review later today.
Mon, Jun 18
Did you miss this comment from my previous review?
Sun, Jun 17
Sat, Jun 16
Jun 15 2018
Jun 12 2018
IIRC, sharing uniqued metadata across modules should only happen as a temporary state. It's a trick for when loading modules that will be merged together, to avoid unnecessarily creating multiple copies of the graphs.
Jun 11 2018
Jun 8 2018
Jun 7 2018
May 31 2018
May 30 2018
May 29 2018
May 24 2018
I have a suggestion for how to respond to Peter's feedback, but this LGTM once he's happy.
May 23 2018
The LangRef looks great, thanks for adding it. I have a few nitpicks and a follow-up on the SkipLineComment discussion. I made it most of the way through the AssemblyWriter (and any comments/questions there might apply more generally to the rest)... you may as well respond to those first, and then I can review the rest.
May 22 2018
May 21 2018
Removing the binary header map files is a clear win, but I'd like someone else to approve this.
May 17 2018
May 16 2018
I found a couple more nits to pick while reading the new version of the lock-free code. LGTM once you fix those.
May 15 2018
I haven't had time to give this a full review, but skimming this I noticed that there's no update to llvm/docs/LangRef.rst. Can you document the syntax and semantics there?
Did this eventually go in?
(Somehow I missed Richard's comment when I replied last night, even though it preceded mine by 5 hours...)
May 14 2018
Since you've answered my question about testing, this LGTM after you address the other feedback. Thanks for splitting out the two commits.
The code looks correct to me, although I have a few suggestions inline. When you resubmit, please include full context (e.g., git diff -U999999).
I wonder if there's some way to write a test for this. E.g., a unit test that registers a signal handler for SIGUSR1 and then does what used to be necessary to trigger a crash.
May 11 2018
May 10 2018
This SGTM, but I wouldn't mind hearing from others. I wonder if this is worth a quick RFC on cfe-dev?
Apr 30 2018
Apr 23 2018
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.
Apr 22 2018
I don't think this is quite right. I know at least make-based incremental builds wouldn't deal well with this.