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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
