<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<br>
<br>
<div class="moz-cite-prefix">On 10/15/2016 12:07 AM, Adrian Prantl
wrote:<br>
</div>
<blockquote
cite="mid:79EF4D03-BFF4-4589-A432-CDDF8BEC38DA@apple.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
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>
</blockquote>
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.<br>
<blockquote
cite="mid:79EF4D03-BFF4-4589-A432-CDDF8BEC38DA@apple.com"
type="cite">
<div class="">
<div class="">- Is an opaque DIAlignment type any more readable
than an explicit uint32_t or unsigned?</div>
</div>
</blockquote>
<blockquote
cite="mid:79EF4D03-BFF4-4589-A432-CDDF8BEC38DA@apple.com"
type="cite">
<div class="">
<div class="">Perhaps having a plain "unsigned AlignmentInBits"
is just as readable and more in line with the surrounding
code.</div>
</div>
</blockquote>
<br>
I think "InBits" is more suitable for name of actual variable (and
it also results in smaller size of patch).<br>
<blockquote
cite="mid:79EF4D03-BFF4-4589-A432-CDDF8BEC38DA@apple.com"
type="cite">
<div class="">
<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 moz-do-not-send="true"
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 moz-do-not-send="true"
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 moz-do-not-send="true"
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 moz-do-not-send="true"
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 moz-do-not-send="true"
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 moz-do-not-send="true"
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 moz-do-not-send="true"
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 moz-do-not-send="true"
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 moz-do-not-send="true"
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>
</blockquote>
<br>
<pre class="moz-signature" cols="72">--
Best Regards,
Victor</pre>
</body>
</html>