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(
----------------
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?
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
