[PATCH] D43248: [Attr] Fix parameter indexing for attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D43248#1035477, @jdenny wrote:

> In https://reviews.llvm.org/D43248#1035466, @aaron.ballman wrote:
>
> > It seems like there are some other changes than just the serialize and deserialize that I'm not opposed to, but am wondering why they're needed. It seems some functions are now `getFoo()` calls
>
>
> These were originally named getFoo, and my previous patch changed them to foo.  I believe I did that to make ParamIdxArgument accessors more like VariadicParamIdxArgument accessors (which inherits accessors from VariadicArgument), but I probably shouldn't have done that.  In any case, this new revision implements ParamIdxArgument using SimpleArgument, and that names accessors like getFoo.


Ahhh, thank you for the explanation, that makes sense.

>> and it seems like some declarations moved around. Are those intended as part of this patch?
> 
> Are you referring to the changes in SemaDeclAttr.cpp?  Those changes are needed because the ParamIdx constructor now asserts that Idx is one-origin, but that requires validating that it's actually one-origin beforehand.  Sorry, I should've mentioned the new asserts.

Ah, okay, thank you!



================
Comment at: cfe/trunk/test/Frontend/ast-attr.cpp:1-2
+// %S/../Sema/attr-print.cpp exercises many different attributes, so we reuse
+// it here to check -emit-ast for attributes.
+
----------------
jdenny wrote:
> aaron.ballman wrote:
> > Can you move this below the RUN line?
> Sure.  I'm still trying to learn the LLVM coding standards.  Is this specified there?
Nope! I'm just used to looking at the very first line of the test to know what it's running, and that seems consistent with other tests.


https://reviews.llvm.org/D43248