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.


cfe-commits mailing list

Reply via email to