- User Since
- Jan 31 2016, 7:15 AM (133 w, 1 d)
Sat, Aug 18
Mon, Aug 13
Looks good to me.
Thu, Aug 9
Looks good, thanks!
- I guess a possible alternative would have been to set LargeCodeModel to true for kernel case? Regardless, I'd prefer being explicit to prevent confusion.
- I'm slightly worried about the default value being CodeModel::Small in case other parts in MCObjectFileInfo start relying on this, but it's definitely the most sensible.
Wed, Aug 8
Thanks Paul! One nit, otherwise this LGTM.
This looks really cool :-)
Add comment to tests explaining why this works on Darwin.
Thanks for the review Stefan!
Updated to use the ReadContext. If the error message doesn't matter that much then I agree this is better.
Tue, Aug 7
Fri, Aug 3
Regarding the naming: I like PubnameKind because of the aforementioned consistency (we also have AccelTableKind. Can't we use an enum class to prevent collisions?
Thu, Aug 2
In the differential you linked we agreed to have the API include an error code as an argument. That still leaves us the option to pass an inconvertibleErrorCode() as the first argument.
Wed, Aug 1
Landed alternative from D49679
Tue, Jul 31
- Address Dave's feedback
The consensus is that we won't support this, at least for now.
LGTM, Thanks for catching this!
I agree with Pavel, but I also don't mind having this test in the meantime. LGTM if you add a FIXME with the direction we want to go.
Sun, Jul 29
Thu, Jul 26
Third time's the charm!
Lang's suggestion was indeed what I had in mind. Thanks!
Wed, Jul 25
Please run the test through llvm-as | llvm-dis, this will fix the numbering of the DI nodes.
Can you add a test for the case where no Vals are provided? I remember something about that requiring an overload without the variadic template arguments.
Thanks Adam, the change looks good but would need a test indeed. Currently the codes are not printed if they are not known, so maybe by adding them they are now printed in one of the existing tests? It would be fine to add a new test if we can't reuse an existing one.
Given these numbers I think we can land this patch as is and figure out the IDP stuff on the mailing list and/or a future patch. LGTM. Thanks Stefan!
- Address Dave's feedback
Tue, Jul 24
Address Pavel's feedback:
- Specialize std::reverse_iterator for llvm::DWARFDie as suggested by Pavel.
Looks good to me but I'll defer to Greg and Dave who had actual feedback.
- Feedback Pavel
Mon, Jul 23
Other than the DIDumpOptions comment (which I think you might have missed) this LGTM.
- Ignore modules for now as discussed offline.
- Document this decision.
I've put up https://reviews.llvm.org/D49679 as an alternative to this patch. Personally I don't like the caveat that you can't store the DWARFDie the iterators point to, but on the other hand I don't like returning by value either. Please let me know which you prefer.
Jul 20 2018
Looks good to me, but I'll defer to Paul
Thanks, I meant to use isOSBinFormatELF but I think it got lost in an accidental undo. The test didn't capture that because it's using a linux target triple.
Please see PR38190 for more details.
Jul 19 2018
I'm happy with this, LGTM!
LGTM. This is definitely much better!