aaron.ballman added a comment. In D127762#4515055 <https://reviews.llvm.org/D127762#4515055>, @sdesmalen wrote:
> Gentle ping @aaron.ballman and @erichkeane. I've addressed all comments and > suggestions and believe we found agreement on the approach (to use a keyword > instead of an attribute). > Are you happy to accept the patch? I think things are looking close, still some minor issues to correct though. ================ Comment at: clang/include/clang/AST/Type.h:3987 /// [implimits] 8 bits would be enough here. - uint16_t NumExceptionType = 0; + uint16_t NumExceptionType; + ---------------- I think this should also be an `unsigned` bit-field so that it packs together with the new field added below. Otherwise we're liable to end up with padding between this field and the next bit-field (which will take a full allocation unit anyway). How about `unsigned NumExceptionType : 10;`? We're reducing the count by 6 bits, but.... I struggle to imagine code being impacted and we're still allowing more than the implementation limits require. ================ Comment at: clang/include/clang/AST/Type.h:4172-4174 bool Variadic : 1; bool HasTrailingReturn : 1; + unsigned AArch64SMEAttributes : 6; ---------------- IIRC, at least one of the compilers used to build Clang has issues with adjacent bit-fields of differing types, so we'd usually make these `bool` fields be `unsigned` at this point to ensure they pack together properly. ================ Comment at: clang/include/clang/AST/Type.h:4204-4205 + else + AArch64SMEAttributes = + (AArch64SMEAttributes ^ Kind) & AArch64SMEAttributes; } ---------------- Isn't the more idiomatic form of this written as `AArch64SMEAttributes &= ~Kind;`? ================ Comment at: clang/include/clang/Basic/AttrDocs.td:6623 + +* the function may be entered in either normal mode (PSTATE.SM=0) or + in streaming mode (PSTATE.SM=1). ---------------- rsandifo-arm wrote: > How about “non-streaming” rather than “normal”? non-streaming sounds good to me. ================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3608-3609 +def err_sme_attr_mismatch : Error< + "function declared '%0' was previously declared '%1'" + " with different SME function attributes">; def err_cconv_change : Error< ---------------- rsandifo-arm wrote: > Also, remove the single quotes around %0 and %1 -- you're getting duplicate quotes in the test cases because of them. ================ Comment at: clang/lib/Sema/SemaType.cpp:7937 + + const FunctionProtoType *FnTy = unwrapped.get()->getAs<FunctionProtoType>(); + if (!FnTy) { ---------------- Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D127762/new/ https://reviews.llvm.org/D127762 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits