aaron.ballman added a comment. > @aaron.ballman, are you actually interested in reviewing these attributes? > 'Cause George added you on his reviews but i totally understand that these > are fairly exotic.
Yeah, I'd like to review these attributes -- thanks for double-checking with me! ================ Comment at: clang/include/clang/Basic/Attr.td:1310 +def MIGServerRoutine : InheritableAttr { + let Spellings = [Clang<"mig_server_routine">]; ---------------- Should this be limited to specific targets, or is this a general concept that applies to all targets? ================ Comment at: clang/include/clang/Basic/Attr.td:1312 + let Spellings = [Clang<"mig_server_routine">]; + let Subjects = SubjectList<[Function]>; + let Documentation = [MIGCallingConventionDocs]; ---------------- Objective-C methods as well? ================ Comment at: clang/include/clang/Basic/Attr.td:1313 + let Subjects = SubjectList<[Function]>; + let Documentation = [MIGCallingConventionDocs]; +} ---------------- This isn't really a calling convention though, is it? It doesn't change codegen, impact function types, or anything like that. ================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8702 +def warn_mig_server_routine_does_not_return_kern_return_t : Warning< + "'%0' attribute only applies to functions that return a kernel return code">, + InGroup<IgnoredAttributes>; ---------------- Will users understand "kernel return code"? Should this say `kern_return_t` explicitly? No need to use %0 here, just spell out the attribute name directly (unless you expect this to be used by multiple attributes, in which case the name of the diagnostic should be changed). ================ Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7142 + + // Mach Interface Generator annotations. + case ParsedAttr::AT_MIGServerRoutine: ---------------- This comment doesn't add a ton of value, I'd probably remove it. ================ Comment at: clang/test/Sema/attr-mig.c:6 + +__attribute__((mig_server_routine)) kern_return_t var = KERN_SUCCESS; // expected-warning-re{{'mig_server_routine' attribute only applies to functions{{$}}}} + ---------------- Can you also add a test showing that the attribute doesn't accept arguments. ================ Comment at: clang/test/Sema/attr-mig.c:17 +} + +kern_return_t bar_forward() { // no-warning ---------------- Here's an edge case to consider: ``` __attribute__((mig_server_routine)) int foo(void); typedef int kern_return_t; kern_return_t foo(void) { ... } ``` Do you have to care about situations like that? ================ Comment at: clang/test/Sema/attr-mig.cpp:10 +public: + virtual __attribute__((mig_server_routine)) IOReturn externalMethod(); + virtual __attribute__((mig_server_routine)) void anotherMethod(); // expected-warning{{'mig_server_routine' attribute only applies to functions that return a kernel return code}} ---------------- Can you use the C++ spelling for the attribute, so we have a bit of coverage for that? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58365/new/ https://reviews.llvm.org/D58365 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits