aaron.ballman added inline comments.
================
Comment at: include/clang/Basic/Attr.td:1230
+ let Spellings = [GCC<"alloc_align">];
+ let Subjects = SubjectList<[ Function]>;
+ let Args = [IntArgument<"ParamIndex">];
----------------
There's a spurious space between [ and Function.
If we want to behave like GCC, then your subject list is fine. If we want to
tighten up what we allow rather than silently accept the attribute and do
nothing with it, this should probably be HasFunctionProto because the attribute
doesn't seem to make sense on an unprototyped function.
================
Comment at: include/clang/Basic/Attr.td:1224
+def AllocAlign : InheritableAttr {
+ let Spellings = [ GCC<"alloc_align"> ];
+ let Subjects = SubjectList<[Function]>;
----------------
erichkeane wrote:
> aaron.ballman wrote:
> > Extra spaces in the declaration that do not match the style of the rest of
> > the file (same happens below).
> Strangely, this is ClangFormat at work :/
Ah, I guess clang-format doesn't quite understand .td files.
================
Comment at: include/clang/Basic/AttrDocs.td:252
+declaration to specify that the return value of the function (which must be a
+pointer type) has an alignment specified by the indicated parameter, starting
+with 1.
----------------
aaron.ballman wrote:
> I would split the "starting with 1" off into its own (full) sentence for
> clarity purposes. Also, please spell out one instead of 1.
>
> You may also want to clarify how member functions do/do not impact this index.
This does not appear to have been handled?
================
Comment at: lib/CodeGen/CGCall.cpp:4374
+ } else if (const auto *AA = TargetDecl->getAttr<AllocAlignAttr>()) {
+ llvm::Value *ParamVal = IRCallArgs[AA->getParamIndex() - 1];
+ EmitAlignmentAssumption(Ret.getScalarVal(), ParamVal);
----------------
erichkeane wrote:
> aaron.ballman wrote:
> > Instead of hoping all of the callers of `getParamIndex()` will remember
> > that this is a weird one-based thing, why not give the semantic attribute
> > the correct index when attaching the attribute to the declaration?
> I played with this for a while, and the difficulty is that AddAllocAlignAttr
> requires the 1-based index, since it needs to error based on that number.
> Additionally, decrementing the value BEFORE that function becomes difficult,
> since it is an Expr object at that time (which would get messy in the
> template case).
>
> I cannot alter it in the AddAllocAlignAttr function, since that function
> actually gets called TWICE in the template instantiation case, so I'd likely
> need some sort of flag that told whether to treat it as decremented or not.
> In general, this gets really ugly really quickly.
>
> Basically, I don't see a good place to decrement the value anywhere without
> causing a much more significant change.
Thank you for the explanation, that makes sense to me.
================
Comment at: lib/Sema/SemaDeclAttr.cpp:1599
+ int IndexVal;
+ if (!checkPositiveIntArgument(*this, TmpAttr, ParamExpr, IndexVal,
+ /*Index=*/1))
----------------
erichkeane wrote:
> aaron.ballman wrote:
> > It seems strange to me that you check that it's a positive integer argument
> > before checking the param is an integer type.
> >
> > Why not use `checkFunctionOrMethodParameterIndex()`?
> I'm unaware of checkFunctionOrMethodParameterIndex, there are a ton of odd
> free fucntions around here, I just copied from some of the surrounding
> functions.
>
> That said, these two poorly named functions are actually checking 2 different
> pieces of data. So in the case of:
> void func(int foo) __attribute__((alloc_align(1));
>
> The "checkPositiveIntArgument" checks to ensure that '1' is a positive
> integer. "checkParamIsIntegerType" checks that "foo" (the corresponding
> parameter) is an integer, and that '1' is in range.
Ah, I see (and yes, that function name is rather ambiguous). I think you should
be using `checkFunctionOrMethodParameterIndex()` in place of
`checkPositiveIntArgument()` and leave in the `checkParamIsIntegerType()`.
https://reviews.llvm.org/D29599
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
