<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">The important change in this review is going from uint64_t -> uint32_t for the alignment.<div class="">This is what we should be discussing here :-)</div><div class=""><br class=""><div class="">As for the typedef I think it boils down to bikeshedding:</div><div class="">- Is the typedef giving us more type safety? (looks like the answer is no)</div><div class="">- Is an opaque DIAlignment type any more readable than an explicit uint32_t or unsigned?</div><div class="">Perhaps having a plain "unsigned AlignmentInBits" is just as readable and more in line with the surrounding code.</div><div class=""><br class=""></div><div class="">-- adrian</div><div class=""><br class=""></div><div class=""><div><blockquote type="cite" class=""><div class="">On Oct 14, 2016, at 1:54 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="">dblaikie@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class="">(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)</div><br class=""><div class="gmail_quote"><div dir="ltr" class="">On Fri, Oct 14, 2016 at 1:51 PM Adrian Prantl <<a href="mailto:aprantl@apple.com" class="">aprantl@apple.com</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br class="gmail_msg">
> On Oct 14, 2016, at 12:02 PM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com" class="gmail_msg" target="_blank">dexonsmith@apple.com</a>> wrote:<br class="gmail_msg">
><br class="gmail_msg">
>><br class="gmail_msg">
>> On 2016-Oct-14, at 10:16, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="gmail_msg" target="_blank">dblaikie@gmail.com</a>> wrote:<br class="gmail_msg">
>><br class="gmail_msg">
>> 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)<br class="gmail_msg">
>><br class="gmail_msg">
>> On Fri, Oct 14, 2016 at 10:15 AM Adrian Prantl <<a href="mailto:aprantl@apple.com" class="gmail_msg" target="_blank">aprantl@apple.com</a>> wrote:<br class="gmail_msg">
>>> On Oct 14, 2016, at 10:13 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="gmail_msg" target="_blank">dblaikie@gmail.com</a>> wrote:<br class="gmail_msg">
>>><br class="gmail_msg">
>>><br class="gmail_msg">
>>><br class="gmail_msg">
>>> On Fri, Oct 14, 2016 at 9:59 AM Adrian Prantl <<a href="mailto:aprantl@apple.com" class="gmail_msg" target="_blank">aprantl@apple.com</a>> wrote:<br class="gmail_msg">
>>><br class="gmail_msg">
>>><br class="gmail_msg">
>>>> On Oct 14, 2016, at 9:48 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="gmail_msg" target="_blank">dblaikie@gmail.com</a>> wrote:<br class="gmail_msg">
>>><br class="gmail_msg">
>>>><br class="gmail_msg">
>>><br class="gmail_msg">
>>>> 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.<br class="gmail_msg">
>>><br class="gmail_msg">
>>>><br class="gmail_msg">
>>><br class="gmail_msg">
>>><br class="gmail_msg">
>>><br class="gmail_msg">
>>> This came out of the review thread in:<br class="gmail_msg">
>>><br class="gmail_msg">
>>><br class="gmail_msg">
>>><br class="gmail_msg">
>>> D25073: [DebugInfo]: preparation to implement DW_AT_alignment<br class="gmail_msg">
>>><br class="gmail_msg">
>>> <a href="https://reviews.llvm.org/D25073" rel="noreferrer" class="gmail_msg" target="_blank">https://reviews.llvm.org/D25073</a><br class="gmail_msg">
>>><br class="gmail_msg">
>>><br class="gmail_msg">
>>><br class="gmail_msg">
>>> 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.<br class="gmail_msg">
>>><br class="gmail_msg">
>>> 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?)<br class="gmail_msg">
>><br class="gmail_msg">
>> 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.<br class="gmail_msg">
><br class="gmail_msg">
> 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?<br class="gmail_msg">
<br class="gmail_msg">
You are right, not as a typedef. We would have to wrap it in a class to benefit from the type safety.<br class="gmail_msg">
<br class="gmail_msg">
>><br class="gmail_msg">
>> -- adrian<br class="gmail_msg">
>><br class="gmail_msg">
>>><br class="gmail_msg">
>>><br class="gmail_msg">
>>><br class="gmail_msg">
>>><br class="gmail_msg">
>>> -- adrian<br class="gmail_msg">
<br class="gmail_msg">
</blockquote></div>
</div></blockquote></div><br class=""></div></div></body></html>