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