[PATCH] D58365: [attributes] Add a MIG server routine attribute.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
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>;
----------------
NoQ wrote:
> aaron.ballman wrote:
> > 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).
> It should say either `kern_return_t` or `IOReturn` depending on the specific framework that's being used (the latter is a typedef for the former). I guess i could scan the AST to for a `typedef kern_return_t IOReturn` and display the appropriate message, but this sort of stuff always sounds like an overkill. For now i change the wording to an exact "a kern_return_t". I could also say "a kern_return_t or an IOReturn", do you have any preference here?
Yeah, I think that scanning the AST would be overkill. This seems sufficiently clear, and we can always improve it if users wind up being confused in practice.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6407
+    ASTContext &ACtx = S.getASTContext();
+    QualType T = AnyCall::forDecl(D)->getReturnType(ACtx);
+    bool IsKernReturnT = false;
----------------
I'd prefer this to use `getFunctionOrMethodResultType()`.


================
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, Objective-C methods, and blocks{{$}}}}
+
----------------
What's the purpose of the `{{$}}`?


================
Comment at: clang/test/Sema/attr-mig.c:17
+}
+
+kern_return_t bar_forward() { // no-warning
----------------
NoQ wrote:
> aaron.ballman wrote:
> > 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?
> > Do you have to care about situations like that?
> 
> I hope i not :) `kern_return_t` is available pretty much everywhere and most likely it's not a problem to update the first declaration of `foo()` with `kern_return_t`. Ok if i add a relaxing code later if it turns out that i have to?
That's what I was hoping to hear. :-)


================
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}}
----------------
NoQ wrote:
> aaron.ballman wrote:
> > Can you use the C++ spelling for the attribute, so we have a bit of coverage for that?
> Is there a vision that i should also provide a namespaced C++ attribute, eg. `[[mig::server_routine]]`?
I think this should continue to use `[[clang::mig_server_routine]]`.


================
Comment at: clang/test/Sema/attr-mig.m:21
+
+  // TODO: Warn that this block doesn't return a kern_return_t.
+  void (^invalid_block)() = ^ __attribute__((mig_server_routine)) {};
----------------
I'd change this to use FIXME instead of TODO.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58365/new/

https://reviews.llvm.org/D58365