- User Since
- Oct 15 2012, 2:12 PM (317 w, 1 d)
I can sign off on this.
This looks better to me. I've got an inline request for some elaboration and you might want to let chandlerc comment, but otherwise OK with me.
Mon, Nov 12
Fri, Nov 9
I think this is the right direction, couple comment:
Thu, Nov 8
Once we get everything else handled this is fine :)
It'd be really great if we could handle all of this in the mips backend rather than in general code.
The llvm backend patch here has discussion around debug info kinds that we should iron out first.
Some inline comments - I'm still not happy with some of how this patch is and would like to see some changes and elaborations of how we split things out. Mostly it's bikeshed naming things, but the current state is a bit more confusing than without.
Some drive by comments again. Be good to get @ributzka to take a final look.
I have no other bikeshed colors here. :)
Naming sounds ok to me :)
Tue, Nov 6
Mon, Nov 5
Tue, Oct 30
Random drive-by bikeshed:
Mon, Oct 29
Go ahead and commit for them.
Wed, Oct 24
All of the target specific stuff looks fine to me. I'm going to defer to rnk about the windows side of things and aaron for the attributes.
Tue, Oct 23
Fri, Oct 19
LGTM. This code needs a lot of cleanup, but that's for another day.
Feel free to make any follow on changes you need here.
I think Adrian has looked at this more recently than I have. Adding him here.
The specific use of ENUM_ENT is also somewhat confusing, but you didn't add that so OK.
Thu, Oct 18
Oct 13 2018
I'm ok with the "here are a bunch of fuchsia tests" file :)
LGTM as well. Thanks!
Oct 9 2018
Oct 1 2018
Couple of comments/questions:
Sep 24 2018
Sounds like everyone is in favor. Hitting the accept button for Jordan :)
Sep 21 2018
Sep 20 2018
Sep 19 2018
Sep 18 2018
Adding Steve here in case he wants to opine.
Sep 16 2018
Sep 14 2018
Sep 6 2018
Sep 5 2018
I'll get it tomorrow if no one does before.
Meta comment: please split the rename out. I don't mind the rename, but it makes the change much harder to review.
Sep 4 2018
Do you mean basically just line tables or something else? (Minus the need in normal dwarf for a minimal cu to associate the line table with). Also, how does this handle inlining etc?
The change in name here from "line tables" to "directives only" feels a bit confusing. "Limited" seems to be a bit more clear, or even remaining line tables only. Can you explain where you were going with this particular set of changes in a bit more detail please?
Some inline comments.
Rather than testing for isNVPTX in AsmPrinter.cpp I'd rather just make a function "emitPreFunctionDebugInfo" and have it do nothing unless it's NVPTX and then define this in the nvptx backend. Easier to update if nvidia ever fixes this weirdness too.
Aug 30 2018
LGTM, but let's get Stephen to ack as well.
Aug 28 2018
Feel free to add any tests like this that test existing behavior without review in the future. If we want to change the behavior we should probably review that though :)
Yeah, that works for me. If we start getting something wrong it's an edge case we should probably document :)
Aug 24 2018
Going to guess you need someone to commit this for you.
Should we just have them mean the same thing and change it based on target?
Aug 22 2018
Oh, you probably want to fix the commit message here.
Aug 21 2018
Aug 7 2018
Aug 6 2018
LGTM. One inline comment.
Aug 5 2018
Looks pretty good. Could you pass CGM in and just make the functions static I couldn't see any other class variables, but might have missed something. One inline comment as well.