steffenlarsen 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(
----------------
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.
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
