[PATCH] D25620: DebugInfo: introduce DIAlignment type

[Victor Leschuk via llvm-commits]

On 10/15/2016 12:07 AM, Adrian Prantl wrote:
> The important change in this review is going from uint64_t -> uint32_t 
> for the alignment.
> This is what we should be discussing here :-)
>
> As for the typedef I think it boils down to bikeshedding:
> - Is the typedef giving us more type safety? (looks like the answer is no)
Maybe we could use "enum class AlignmentInBits : uint32_t {};" here? 
That will lead in few static_cast<> in BitcodeReader however will result 
in stronger typing.
> - Is an opaque DIAlignment type any more readable than an explicit 
> uint32_t or unsigned?
> Perhaps having a plain "unsigned AlignmentInBits" is just as readable 
> and more in line with the surrounding code.

I think "InBits" is more suitable for name of actual variable (and it 
also results in smaller size of patch).
>
> -- adrian
>
>> On Oct 14, 2016, at 1:54 PM, David Blaikie <dblaikie at gmail.com 
>> <mailto:dblaikie at gmail.com>> wrote:
>>
>> (ah, I just assumed from the number of files changed/the description 
>> this was more than just adding a typedef - *shrug* I'm less fussed 
>> about it, not sure it's worth having a separate typedef for it, but 
>> not enough to have any opinion on this code review)
>>
>> On Fri, Oct 14, 2016 at 1:51 PM Adrian Prantl <aprantl at apple.com 
>> <mailto:aprantl at apple.com>> wrote:
>>
>>
>>     > On Oct 14, 2016, at 12:02 PM, Duncan P. N. Exon Smith
>>     <dexonsmith at apple.com <mailto:dexonsmith at apple.com>> wrote:
>>     >
>>     >>
>>     >> On 2016-Oct-14, at 10:16, David Blaikie <dblaikie at gmail.com
>>     <mailto:dblaikie at gmail.com>> wrote:
>>     >>
>>     >> Victor - was there any particular benefit/motivation for
>>     adding a new type here that you had in mind/had discovered in
>>     implementing it? (or precedent you were inspired by - I admit to
>>     not having all the DI* hierarchy paged in these days, so perhaps
>>     I've missed some obvious prior art)
>>     >>
>>     >> On Fri, Oct 14, 2016 at 10:15 AM Adrian Prantl
>>     <aprantl at apple.com <mailto:aprantl at apple.com>> wrote:
>>     >>> On Oct 14, 2016, at 10:13 AM, David Blaikie
>>     <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
>>     >>>
>>     >>>
>>     >>>
>>     >>> On Fri, Oct 14, 2016 at 9:59 AM Adrian Prantl
>>     <aprantl at apple.com <mailto:aprantl at apple.com>> wrote:
>>     >>>
>>     >>>
>>     >>>> On Oct 14, 2016, at 9:48 AM, David Blaikie
>>     <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
>>     >>>
>>     >>>>
>>     >>>
>>     >>>> Could someone point me to where the discussion for adding
>>     this type came out of? I didn't spot it at a cursory glance of
>>     the previous/existing threads.
>>     >>>
>>     >>>>
>>     >>>
>>     >>>
>>     >>>
>>     >>> This came out of the review thread in:
>>     >>>
>>     >>>
>>     >>>
>>     >>> D25073: [DebugInfo]: preparation to implement DW_AT_alignment
>>     >>>
>>     >>> https://reviews.llvm.org/D25073
>>     >>>
>>     >>>
>>     >>>
>>     >>> where I argued that we should not be using a full uint64_t
>>     for the alignment fields in the DI.* metadata nodes. I don't
>>     think the concrete solution of introducing a new alignment type
>>     has been discussed here before.
>>     >>>
>>     >>> Any particular benefits of introducing a new type? We don't I
>>     think have any wrapper types for otherwise singular numeric
>>     values, do we? (except maybe DIFlags?)
>>     >>
>>     >> I don't have a strong opinion about adding a new type. If we
>>     also had a separate type for sizes, we could potentially make the
>>     interface more typesafe to avoid accidentally confusing size and
>>     alignment, but that's about it.
>>     >
>>     > Haven't looked at the patch, so forgive me if I'm confused, but
>>     I thought you said this was just a typedef.  In that case, how is
>>     it providing any type safety?
>>
>>     You are right, not as a typedef. We would have to wrap it in a
>>     class to benefit from the type safety.
>>
>>     >>
>>     >> -- adrian
>>     >>
>>     >>>
>>     >>>
>>     >>>
>>     >>>
>>     >>> -- adrian
>>
>

-- 
Best Regards,
Victor

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161015/7ffb05ee/attachment.html>