- User Since
- Dec 9 2012, 11:41 PM (405 w, 6 d)
Thu, Sep 17
Wed, Sep 16
Change diagnostics for conflicting swift_name, add a test case
Address everything but warning
Address feedback from @aaron.ballman
I think that tracking the segment length should be okay. It’s a total overhead of 8 bytes, which is only allocated once, however, the naming here is egregiously bad. This is the segment length and not the DSO length nor the DSO mapped length (which may actually be larger or smaller based on the load segment layout).
Tue, Sep 15
Mon, Sep 14
- make the attribute inheritable
- add a test case for inheritance
- add a test case for C++ type alias
- Add additional test case that was requested
Address some feedback from @aaron.ballman
Add additional test case
Sun, Sep 13
Fri, Sep 11
The make based build was long ago deprecated.
Discussed this with @aaron.ballman offline; he is okay with this since this is not going to permit incorrect code through without a diagnostic, even if it is not the most clear and the attribute wont be accidentally ignored. We can add more diagnostics as a follow up if it is useful.
Thu, Sep 10
Address @gribozavr2's comments
Wed, Sep 9
Address feedback from @aaron.ballman
Yes, sorry I wasn't clear about that, but you understood correctly. I was suggesting using -1 as the search all, since it doesn't make sense as a DSO base. Minor comment about a possible constexpr improvement, but doesn't require a re-review.
I was thinking more the SjLj backend. I am okay with an additional option, but this currently would force enable the SEH backend.
Tue, Sep 8
Is there a funnel point that we know that all the rebase entries will pass through to ensure that we catch them rather than adding them at a couple of sites?
It is definitely possible to disable SEH via /EHs- though
Why not use -1 as the indicator instead of 0?
This seems relatively harmless, not sure if its valuable though.
- correct typos
- apply clang-format changes
- const correct a few declarations.
The change itself is fine, but what about downstream projects which define the option? Will that trigger a warning?
Fri, Sep 4
LGTM with the name change.
Thu, Aug 27
I don't think that this is entirely true. I believe it is possible to disable SEH with cl, so I think that it is better to conditionalize it as if defined(__SEH__) || defined(_SEH) since IIRC, _SEH is the define that cl uses.
Aug 21 2020
Sounds good to me; LGTM with the rephrased commit message.
Please ensure that the fix goes into 11 before release (CC: @hans), but the remainder of the change is fine.
Aug 20 2020
Can you please verify that findUnwindSectionsByPhdr was not exposed previously? This seems like it could be an ABI break.
I think that the confusing part is that it is not that they are C functions, they are still assembly, but with C calling convention. I think that you should state that in the description as it currently reads as though they are written in C, which they are not.
Aug 17 2020
The idea itself is reasonable. The summary description is very confusing would you mind reworking that? Could you check what happens for the jumpto member? Ideally it would just be an alias for the function to avoid a frame setup and unnecessary jump. I think that is the only thing that really stands out of needing work in the implementation.
Aug 11 2020
LGTM with the renaming of the local variable. Thanks for implementing this (it was the next item I wanted to add)!
Aug 10 2020
Aug 6 2020
Aug 5 2020
This looks fantastic! I'm really liking the better diagnostics. The only thing I'd change is the "symbol kind %0 unsupported on COFF targets". I think that "relocation variant" is more precise than "symbol kind". Thanks for improving this.
Aug 4 2020
Aug 3 2020
Given that the original motivation here was to help with NetBSD, and the submodule visibility flag handling unblocks NetBSD, I think that I will hold off on this unless some other motivation arises.