[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:
> 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?


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

https://reviews.llvm.org/D114439