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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4202
+  // If the first argument is value dependent we delay setting the arguments.
+  if (AllArgs.size() && AllArgs[0]->isValueDependent()) {
+    auto *Attr = AnnotateAttr::CreateWithDelayedArgs(
----------------
steffenlarsen wrote:
> erichkeane wrote:
> > steffenlarsen wrote:
> > > steffenlarsen wrote:
> > > > erichkeane wrote:
> > > > > if AllArgs.size() == 0, this is an error case.  Annotate requires at least 1 parameter.
> > > > I removed the check but thinking about it again I think it would be better to reintroduce it and let `HandleAnnotateAttr` call `checkAtLeastNumArgs`. That way it will also complain about missing arguments after template instantiation, e.g. if a lone pack expansion argument evaluates to an empty expression list.
> > > I have reintroduced the check for `AllArgs.size()` here. This means if `AllArgs` is empty it will continue to `Sema::HandleAnnotateAttr` which will check that there are at least one element in `AllArgs` and fail otherwise with a useful diagnostic. This diagnostic will also be issued if an empty parameter pack is given and that is the only argument of the annotation. Note that `checkAtLeastNumArgs` is a member of `ParsedAttr` which isn't available in `Sema::HandleAnnotateAttr`, so it replicates the check. It could potentially be moved, but given the size I don't think it's necessary in this patch.
> > > 
> > > I also added some tests for checking that the diagnostic is issued in the cases (pack expansion and explicitly empty).
> > > 
> > > @erichkeane - What do you think?
> > Hmm... I would expect this to diagnose even in the 'delayed args' case as well.  Otherwise:
> > 
> > template<typename T>
> > [[annotate()]]
> > 
> > is not an error, right?
> Both "delayed args" and immediate instantiation go through the diagnostics check. The check is intentionally in `Sema::HandleAnnotateAttr` as either this or template instantiation (if delayed args) goes through it.
> 
> ```
> template<typename T> [[annotate()]] ...
> ```
> 
> would fail because it does not have any arguments which will be identified here as "delayed args" will be skipped. 
> 
> ```
> template<int... Is> [[annotate(Is...)]] ...
> ```
> 
> will fail once instantiated with no template arguments as the instantiating function will pass an empty list of expressions to `Sema::HandleAnnotationAttr` which will check and fail with the diagnostic. The latter is tested and I can add the former as a case while adding the other requested tests.
Can you make sure both of those have a test?  I missed that the 'no args' case would go right to HandleAnnotateAttr.


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

https://reviews.llvm.org/D114439