steffenlarsen marked 3 inline comments as done.
steffenlarsen added inline comments.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4179
+ MutableArrayRef<Expr *> AllArgs) {
+ assert(AllArgs.size());
+
----------------
erichkeane wrote:
> Does this work with a partial specialization? I'd like to see some tests
> that ensure we work in that case (both where the partial specialization sets
> the string literal correctly, and where it doesnt).
I have added such tests in `clang/test/SemaTemplate/attributes.cpp`. Let me
know if they cover the scenario you were thinking about.
================
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:
> > 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.
For sure. It has been added.
================
Comment at: clang/test/Parser/cxx0x-attributes.cpp:276
+ void fiz [[clang::annotate("F", Is..., 1, 2, 3)]] ();
+ void fir [[clang::annotate("G", 1, Is..., 2, 3)]] ();
+}
----------------
steffenlarsen wrote:
> aaron.ballman wrote:
> > We should also be adding test coverage for things like redeclarations,
> > partial specializations, etc to make sure that the behavior there is
> > correct. Those would be Sema tests rather than Parser tests, however.
> Good point. I will add these tests ASAP.
I have added such tests in `clang/test/SemaTemplate/attributes.cpp`. Are these
along the line of what you were thinking?
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
