On Mon, Jul 21, 2014 at 12:36 PM, Hal Finkel <[email protected]> wrote: > ----- Original Message ----- >> From: "Aaron Ballman" <[email protected]> >> To: "Hal Finkel" <[email protected]> >> Cc: "Richard Smith" <[email protected]>, "llvm cfe" >> <[email protected]>, >> [email protected] >> Sent: Monday, July 21, 2014 11:33:17 AM >> Subject: Re: [PATCH] Support the assume_aligned function attribute >> >> On Mon, Jul 21, 2014 at 12:25 PM, Hal Finkel <[email protected]> wrote: >> > ----- Original Message ----- >> >> From: "Aaron Ballman" <[email protected]> >> >> To: [email protected] >> >> Cc: "Hal Finkel" <[email protected]>, "Richard Smith" >> >> <[email protected]>, "llvm cfe" <[email protected]> >> >> Sent: Monday, July 21, 2014 11:22:01 AM >> >> Subject: Re: [PATCH] Support the assume_aligned function attribute >> >> >> >> On Sun, Jul 20, 2014 at 7:25 PM, Richard Smith >> >> <[email protected]> wrote: >> >> > Somewhat unrelated to this patch in particular, but we should >> >> > add a >> >> > UBSan check to catch failed assumptions. >> >> > >> >> > ================ >> >> > Comment at: include/clang/Basic/Attr.td:867 >> >> > @@ +866,3 @@ >> >> > + "ExpectedFunctionOrMethod">; >> >> > + let Args = [IntArgument<"Alignment">, >> >> > DefaultIntArgument<"Offset", 0>]; >> >> > + let Documentation = [Undocumented]; >> >> > ---------------- >> >> > These should probably both be `ExprArgument`s, in order to >> >> > support >> >> > dependent alignment/offset expressions in templates. >> >> > >> >> > ================ >> >> > Comment at: include/clang/Basic/Attr.td:868 >> >> > @@ +867,3 @@ >> >> > + let Args = [IntArgument<"Alignment">, >> >> > DefaultIntArgument<"Offset", 0>]; >> >> > + let Documentation = [Undocumented]; >> >> > +} >> >> > ---------------- >> >> > Please add documentation. >> >> > >> >> > ================ >> >> > Comment at: lib/CodeGen/CGExpr.cpp:3352 >> >> > @@ +3351,3 @@ >> >> > + >> >> > + if (Ret.isScalar() && TargetDecl) >> >> > + if (const AssumeAlignedAttr *AA = >> >> > ---------------- >> >> > Please add parens around this `if`. >> >> > >> >> > ================ >> >> > Comment at: lib/Sema/SemaDeclAttr.cpp:1219-1227 >> >> > @@ +1218,11 @@ >> >> > + >> >> > + if (Attr.getNumArgs() > 2) { >> >> > + S.Diag(Attr.getLoc(), >> >> > diag::err_attribute_too_many_arguments) >> >> > + << Attr.getName() << 2; >> >> > + return; >> >> > + } else if (Attr.getNumArgs() < 1) { >> >> > + S.Diag(Attr.getLoc(), >> >> > diag::err_attribute_too_few_arguments) >> >> > + << Attr.getName() << 1; >> >> > + return; >> >> > + } >> >> > + >> >> > ---------------- >> >> > I think there's a helper function for this sequence of >> >> > checks/diagnostics. If not, there should be. >> >> >> >> They aren't required -- the common handler takes care of that >> >> stuff. >> > >> > No, I put them in specifically because the common handler did not. >> > I noticed because when I was writing the regression tests, if I >> > passed too few parameters (specifically none), the later call to >> > getArg would assert. >> >> Ahhhh shoot, the optional argument is why that's needed (see >> handleCommonAttributeFeatures()). Sorry for leading you astray there; >> I forgot that I hadn't gotten around to implementing optional >> argument >> handlers there yet (which is where this logic really should live). > > Where is that? If you don't get to it first, I'll just implement the correct > logic there.
Just to circle back around on this, I got around to implementing this today in r214407. So *now* you should be able to elide the manual arg count checking with this patch. ~Aaron _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
