[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Sema/ParsedAttr.h:52-57
+  /// The number of non-fake arguments specified in the attribute definition.
+  unsigned NumArgMembers : 4;
   /// True if the parsing does not match the semantic content.
   unsigned HasCustomParsing : 1;
+  // True if this attribute accepts expression parameter pack expansions.
+  unsigned AcceptsExprPack : 1;
----------------
Er, this means the bit packed part of the structure can no longer fit in a 32-bit allocation unit (we went from needing 30 bits to needing 35 bits). That's unfortunate. I don't see any other bits to reasonably steal from.

I took a look to see whether I thought this would have a significant performance impact. We definitely keep arrays of these things around and walk over the array, so I expect some performance loss from this change. But it seems like the sort of thing likely to fall out in the noise rather than be on the hot path, so I think this is fine. However, if we see a noticeable performance regression, we should keep this change in mind to see if there's something more we can do to keep us within a 32-bit allocation unit.


================
Comment at: clang/include/clang/Sema/ParsedAttr.h:113
   }
+  // Check if argument at index I is an expression argument
+  virtual bool isExprArg(size_t N) const { return false; }
----------------
I don't think this API is needed. `isArgExpr()` already exists on the interface and I'm not certain how this differs. (If this API is needed, we need a far better name for it because I'd have no idea how to distinguish `isExprArg()` and `isArgExpr()`.)


================
Comment at: clang/include/clang/Sema/Sema.h:4383-4385
+  bool checkStringLiteralExpr(const AttributeCommonInfo &CI, const Expr *E,
+                              StringRef &Str,
+                              SourceLocation *ArgLocation = nullptr);
----------------
This name makes it sound like far more general of an API than it is. This function is specific to checking string literal arguments for an attribute. But then why does `checkStringLiteralArgumentAttr()` not suffice? It does the same thing, but with more bells and whistles.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4185
   StringRef Str;
-  if (!S.checkStringLiteralArgumentAttr(AL, 0, Str))
+  if (!checkStringLiteralExpr(CI, AllArgs[0], Str))
     return;
----------------
This is a slight loss of functionality. We used to recommend identifiers be converted to string literals under the old interface. I don't think this refactoring is necessary any longer, right?


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4179-4182
+  if (AllArgs.size() < 1) {
+    Diag(CI.getLoc(), diag::err_attribute_too_few_arguments) << CI << 1;
+    return;
+  }
----------------
steffenlarsen wrote:
> aaron.ballman wrote:
> > Please double-check that the call to `checkCommonAttributeFeatures()` from `ProcessDeclAttribute()` doesn't already handle this for you. If it does, then I think this can be replaced with `assert(!AllArgs.empty() && "expected at least one argument");`
> It does go through `checkCommonAttributeFeatures` but (as of the recent changes) it will skip size-checks if arguments are delayed as a pack expansion could potentially populate the seemingly missing expressions after template instantiation for some attribute.
> For annotate we could also have a pack as the only expression, which would then evaluate to an empty list of expressions. Since this path is also taken by `instantiateDependentAnnotationAttr` if there are delayed args. In reality it is only really needed after template instantiations, given as you said `checkCommonAttributeFeatures` will do the checking in the other case, but I personally think it is cleaner to have it here. If you disagree I will move it into `instantiateDependentAnnotationAttr` instead and add the assert.
Ah, I think what caught me off-guard is that I was expecting `checkCommonAttributeFeatures()` to be called again from template instantiation time. Then there are less (possibly even no more) dependent arguments, so it can do more automated checking. However, I see that `Sema::InstantiateAttrs()` is crying out for refactoring to make that more useful, so it's outside the scope of this patch (what you're doing here is fine). Thanks!


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4203
+  if (AllArgs.size() && AllArgs[0]->isValueDependent()) {
+    auto *Attr = AnnotateAttr::CreateWithDelayedArgs(
+        S.getASTContext(), AllArgs.data(), AllArgs.size(), AL);
----------------
steffenlarsen wrote:
> aaron.ballman wrote:
> > erichkeane wrote:
> > > aaron.ballman wrote:
> > > > erichkeane wrote:
> > > > > I would like @aaron.ballman to comment on this, but I think we probably want this case to be covered in the top of `HandleDeclAttr`, which would mean in the 'not all values filled' case, we skip the 'handleAnnotateAttr'.  
> > > > > 
> > > > > WDYT Aaron?  The downside is that this function couldn't check a 'partially filled in' attribute, but it would make us that much closer to this flag being a very simple thing to opt into.
> > > > Do you mean `ProcessDeclAttribute()`? I don't think we should have attribute-specific logic in there, but are you thinking of something more general than that (I'm not seeing how the suggestion makes the flag easier to opt into)?
> > > Ah, yes, thats what I mean.  The question for ME is whether there should be a generic "this supports variadic pack, so check to see if all the named non-expr arguments are fill-in-able.  If not, do the 'delayed' version.
> > > 
> > > This would mean that HandleAnnotateAttr NEVER sees the "CreateWithDelayedArgs" case.
> > Thanks for clarifying -- yes, I think that would be preferable if it works out in a clean, generic way. I'd be fine with tablegen emitting something else (if necessary) to help generalize it.
> `handleAnnotateAttr` is now oblivious to the concept of "delayed arguments". Instead tablegen generates a common handle function (`handleAttrWithDelayedArgs`) which will, based on the given `ParsedAttr` that supports delayed arguments, create and add the corresponding attribute with delayed arguments by calling the corresponding `CreateWithDelayedArgs`. The need for delaying arguments is decided as described in `MustDelayAttributeArguments`.
> 
> Is this approximately what you were thinking?
This is more along the lines of what I had in mind, yes.


================
Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:2312
+
+    // All these spellings take are parsed unevaluated.
+    forEachUniqueSpelling(Attr, [&](const FlattenedSpelling &S) {
----------------
The comment doesn't seem grammatical and may need some expounding.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114439/new/

https://reviews.llvm.org/D114439