haberman added a comment.
I addressed nearly all of the comments. I have just a few more test cases to
add: Obj-C blocks and ARC.
I left one comment open re: a "regular" function. I'll dig into that more when
I am adding the last few test cases.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2832
+def err_musttail_only_from_function : Error<
+ "%0 attribute can only be used from a regular function.">;
+def err_musttail_return_type_mismatch : Error<
----------------
aaron.ballman wrote:
> What is a "regular function?"
I may have been trying to distinguish between blocks, or lambdas, I can't
exactly remember.
I think I still need to add tests for blocks and Obj-C refcounts. I'm going to
leave this comment open for now as a reminder to revisit this.
================
Comment at: clang/lib/CodeGen/CGCall.cpp:5315-5317
+ // TODO(haberman): insert checks/assertions to verify that this early exit
+ // is safe. We tried to verify this in Sema but we should double-check
+ // here.
----------------
aaron.ballman wrote:
> Are you planning to handle this TODO in the patch? If not, can you switch to
> a FIXME without a name associated with it?
I am interested in feedback on the best way to proceed here.
- Is my assessment correct that we should have an assertion that validates
this?
- Is such an assertion reasonably feasible to implement?
- Is it ok to defer with FIXME, or should I try to fix it in this patch?
I've changed it to a FIXME for now.
================
Comment at: clang/lib/Sema/SemaStmt.cpp:561-568
+ for (const auto *A : Attrs) {
+ if (A->getKind() == attr::MustTail) {
+ if (!checkMustTailAttr(SubStmt, *A)) {
+ return SubStmt;
+ }
+ setFunctionHasMustTail();
+ }
----------------
aaron.ballman wrote:
> This functionality belongs in SemaStmtAttr.cpp, I think.
That is where I had originally put it, but that didn't work for templates. The
semantic checks can only be performed at instantiation time.
`ActOnAttributedStmt` seems to be the right hook point where I can evaluate the
semantic checks for both template and non-template functions (with template
functions getting checked at instantiation time).
================
Comment at: clang/lib/Sema/SemaStmt.cpp:620
+ const CXXMethodDecl *CMD = dyn_cast<CXXMethodDecl>(mem->getMemberDecl());
+ assert(CMD && !CMD->isStatic());
+ CalleeThis = CMD->getThisType()->getPointeeType();
----------------
aaron.ballman wrote:
> Is this assertion valid? Consider:
> ```
> struct S {
> static int foo();
> };
>
> int bar() {
> S s;
> [[clang::musttail]] return s.foo();
> }
> ```
I have a test for that in `CodeGen/attr-musttail.cpp` (see `Func4()`). It
appears that this goes through `FunctionToPointerDecay` and ends up getting
handled by the function case below.
================
Comment at: clang/lib/Sema/SemaStmt.cpp:665-666
+ ArrayRef<ParmVarDecl *> caller_params = CallerDecl->parameters();
+ size_t n = CallerDecl->param_size();
+ for (size_t i = 0; i < n; i++) {
+ if (!TypesMatch(callee_params[i], caller_params[i]->getType())) {
----------------
aaron.ballman wrote:
> How do you want to handle variadic function calls?
I added a check to disallow variadic function calls.
================
Comment at: clang/test/Sema/attr-musttail.cpp:17
+long g(int x);
+int h(long x);
+
----------------
aaron.ballman wrote:
> I'd appreciate a test of promotion behavior:
> ```
> int i(int x);
> int s(short x);
>
> int whatever1(int x) {
> [[clang::musttail]] return s(x);
> }
>
> int whatever2(short x) {
> [[clang::musttail]] return i(x);
> }
> ```
Done (my understanding is that these should both fail, since they would violate
the LLVM rules that the caller and callee prototypes must match).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D99517/new/
https://reviews.llvm.org/D99517
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
