[PATCH] align_value attribute in Clang

[Aaron Ballman]

On Thu, Oct 2, 2014 at 5:33 PM, Hal Finkel <hfinkel at anl.gov> wrote:
> ----- Original Message -----
>> From: "Aaron Ballman" <aaron.ballman at gmail.com>
>> To: reviews+D4635+public+b7e82bdc1d8c324a at reviews.llvm.org
>> Cc: "Hal Finkel" <hfinkel at anl.gov>, "Michael Spencer" <bigcheesegs at gmail.com>, "Richard Smith"
>> <richard at metafoo.co.uk>, "Alexey Bataev" <a.bataev at hotmail.com>, "llvm cfe" <cfe-commits at cs.uiuc.edu>, "Алексей
>> Нурмухаметов" <nurmukhametov.alex at gmail.com>
>> Sent: Thursday, October 2, 2014 4:04:07 PM
>> Subject: Re: [PATCH] align_value attribute in Clang
>>
>> On Thu, Oct 2, 2014 at 4:10 PM, hfinkel at anl.gov <hfinkel at anl.gov>
>> wrote:
>> > Alright, after much discussion, I think this is ready.
>> >
>> > To quickly recap, there had been some discussion regarding whether
>> > or not align_value needed to be part of the type system (so that
>> > it would be propagated by template type deduction, for example),
>> > after after receiving feedback from Richard, Alexey, et al., we're
>> > settled on no. This is not part of the type system, and will only
>> > "propagate" through templates, auto, etc. by optimizer deduction
>> > after inlining. This seems consistent with Intel's implementation.
>> >
>> > Aaron, this new revision should account for all additional points
>> > from your last review.
>>
>> Aside from some very minor formatting nits, LGTM!
>>
>> > Index: include/clang/Basic/Attr.td
>> > ===================================================================
>> > --- include/clang/Basic/Attr.td
>> > +++ include/clang/Basic/Attr.td
>> > @@ -354,6 +354,25 @@
>> >    let Documentation = [Undocumented];
>> >  }
>> >
>> > +def AlignValue : Attr {
>> > +  let Spellings = [
>> > +                   // Unfortunately, this is semantically an
>> > assertion, not a
>> > +                   // directive (something else must ensure the
>> > alignment), so
>> > +                   // aligned_value is a probably a better name.
>> > We might want
>> > +                   // to add an aligned_value spelling in the
>> > future (and a
>> > +                   // corresponding C++ attribute), but this can
>> > be done later
>> > +                   // once we decide if we also want them to have
>> > +                   // slightly-different semantics than Intel's
>> > align_value.
>> > +                   GNU<"align_value">
>> > +                   // Intel's compiler on Windows also supports:
>> > +                   // , Declspec<"align_value">
>> > +                  ];
>>
>> This formatting is unlike anything else in the file. Would be good to
>> tighten it up a bit by shifting to be indented under the let.
>
> I adjusted this to what I think you meant. Please feel free to correct it, or have me do so, if it is still quite what you'd like. r218910, thanks!

I reflowed the comments somewhat, but you basically had it right. I
think I'm just nitpicky. ;-)

~Aaron