[PATCH] D43248: [Attr] Fix printing of parameter indices in attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: include/clang/Basic/Attr.td:182
+  // it would always be false.
+  string DisallowImplicitThisParamName = disallowImplicitThisParamName;
+}
----------------
jdenny wrote:
> jdenny wrote:
> > aaron.ballman wrote:
> > > Is there much benefit to forcing the attribute author to pick a name for this? It seems like this is more of a Boolean value: either implicit this is allowed or not. It would be really nice if we could hide these mechanics as implementation details so that the user only needs to write `VariadicParamIndexArgument<"name", DisallowImplicitThis>` (or something similar) when defining the attribute, and ideally not have to do any extra work in SemaDeclAttr.cpp by taking care of it in ClangAttrEmitter.cpp if possible.
> > Thanks for the review.  I'll work on that.
> > Is there much benefit to forcing the attribute author to pick a name for this? It seems like this is more of a Boolean value: either implicit this is allowed or not.
> 
> If the attribute author picks the name, then the attribute author can ensure there's only one of these per attribute.  I could rewrite it to have one per VariadicParamIndexArgument and one per ParamIndexArgument if you like.  In that case, ArgumentWithTypeTagAttr would end up with two of these, and future attributes could potentially have more, but they should all have the same value within a single attribute.  I didn't investigate how that redundancy would actually impact memory usage.  What do you think?
> 
> >  It would be really nice if we could hide these mechanics as implementation details so that the user only needs to write VariadicParamIndexArgument<"name", DisallowImplicitThis> (or something similar) when defining the attribute, and ideally not have to do any extra work in SemaDeclAttr.cpp by taking care of it in ClangAttrEmitter.cpp if possible.
> 
> So far, I haven't found a good way to accomplish that, or maybe I've misunderstood you....
> 
> The logic of checkFunctionOrMethodParameterIndex in SemaDeclAttr.cpp seems pretty tightly coupled with its users' logic.  For example, handleNonNullAttr uses the indices as adjusted by checkFunctionOrMethodParameterIndex to determine which indices belong in the array to be passed to the NonNullAttr constructor.  We could try to have NonNullAttr (in the constructor, presumably) perform the adjustment of indices so that SemaDeclAttr.cpp doesn't need that logic, but then it would be too late to for handleNonNullAttr to filter them.
> 
> The only extra work this patch adds to SemaDeclAttr.cpp beyond what's already there is to reuse the DisallowImplicitThis that is essentially already computed in checkFunctionOrMethodParameterIndex.
> 
> Another possibility is to have SemaDeclAttr.cpp fully encapsulate the index adjustment logic and pass an index offset to attribute constructors, so ClangAttrEmitter.cpp would just know it has to print indices with some given offset.  But a general index offset is wider than the single bool being stored now.  Again, I haven't investigated the actual impact on memory usage.
> 
> Do you see a better way to achieve the encapsulation you're looking for?
> 
> 
> In that case, ArgumentWithTypeTagAttr would end up with two of these, and future attributes could potentially have more, but they should all have the same value within a single attribute. I didn't investigate how that redundancy would actually impact memory usage. What do you think?

That redundancy could probably be worked around in ClangAttrEmitter.cpp by inspecting the attribute argument list and noticing duplicate `[Variadic]ParamIndexArgument` before generating the code for the arguments, perhaps?

> Another possibility is to have SemaDeclAttr.cpp fully encapsulate the index adjustment logic and pass an index offset to attribute constructors, so ClangAttrEmitter.cpp would just know it has to print indices with some given offset.

This was more along the lines of what I was thinking of.

Basically, it seems like the declarative part in Attr.td should be able to specify an attribute argument is intended to be a function parameter positional index, and whether implicit this needs special handling or not. Past that, the semantics of the attribute really shouldn't matter -- when asking for the index, it should be automatically adjusted so that users of the attribute don't have to do anything, and when printing the attribute, the index should be appropriately adjusted back. I'd like to avoid needing collusion between the declarative part in Attr.td and the semantic part in SemaDeclAttr.cpp because that collusion can get out of sync. It feels like we should be able to hide that collusion in ClangAttrEmitter.cpp by using some extra flags on the attribute that get set (automatically) during construction. The way you have things set up right now is very close to this, except the information has to be passed to the constructor manually in SemaDeclAttr after being calculated by checkFunctionOrMethodParameterIndex().

I'm not too worried about the memory usage at this point; if it looks problematic, we can likely address it.


https://reviews.llvm.org/D43248