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

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen marked 3 inline comments as done.
steffenlarsen added inline comments.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4179
+                              MutableArrayRef<Expr *> AllArgs) {
+  assert(AllArgs.size());
+
----------------
erichkeane wrote:
> Does this work with a partial specialization?  I'd like to see some tests that ensure we work in that case (both where the partial specialization sets the string literal correctly, and where it doesnt).
I have added such tests in `clang/test/SemaTemplate/attributes.cpp`. Let me know if they cover the scenario you were thinking about.


================
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(
----------------
erichkeane wrote:
> 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.
For sure. It has been added.


================
Comment at: clang/test/Parser/cxx0x-attributes.cpp:276
+  void fiz [[clang::annotate("F", Is..., 1, 2, 3)]] ();
+  void fir [[clang::annotate("G", 1, Is..., 2, 3)]] ();
+}
----------------
steffenlarsen wrote:
> aaron.ballman wrote:
> > We should also be adding test coverage for things like redeclarations, partial specializations, etc to make sure that the behavior there is correct. Those would be Sema tests rather than Parser tests, however.
> Good point. I will add these tests ASAP.
I have added such tests in `clang/test/SemaTemplate/attributes.cpp`. Are these along the line of what you were thinking?


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

https://reviews.llvm.org/D114439