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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
