On Thu, Oct 2, 2014 at 5:33 PM, Hal Finkel <[email protected]> wrote: > ----- Original Message ----- >> From: "Aaron Ballman" <[email protected]> >> To: [email protected] >> Cc: "Hal Finkel" <[email protected]>, "Michael Spencer" >> <[email protected]>, "Richard Smith" >> <[email protected]>, "Alexey Bataev" <[email protected]>, "llvm cfe" >> <[email protected]>, "Алексей >> Нурмухаметов" <[email protected]> >> 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, [email protected] <[email protected]> >> 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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
