[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Mostly just nits from me, but the attribute portions look good to me.



================
Comment at: clang/include/clang/AST/IgnoreExpr.h:127
+  if (CCE && CCE->isElidable() && !isa<CXXTemporaryObjectExpr>(CCE)) {
+    auto NumArgs = CCE->getNumArgs();
+    if ((NumArgs == 1 ||
----------------



================
Comment at: clang/lib/Sema/SemaStmt.cpp:628
+
+  const CallExpr *CE =
+      dyn_cast_or_null<CallExpr>(IgnoreParenImplicitAsWritten(Ex));
----------------



================
Comment at: clang/lib/Sema/SemaStmt.cpp:636-637
+
+  if (!CE->getCalleeDecl()) {
+    assert(hasUncompilableErrorOccurred() && "expected previous error");
+    return false;
----------------
This worries me slightly -- not all `CallExpr` objects have a callee declaration (https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/Expr.cpp#L1367). That said, I'm struggling to come up with an example that isn't covered so this may be fine.


================
Comment at: clang/lib/Sema/SemaStmt.cpp:641
+
+  if (const ExprWithCleanups *EWC = dyn_cast<ExprWithCleanups>(Ex)) {
+    if (EWC->cleanupsHaveSideEffects()) {
----------------



================
Comment at: clang/lib/Sema/SemaStmt.cpp:655
+
+  const FunctionDecl *CallerDecl = dyn_cast<FunctionDecl>(CurContext);
+
----------------



================
Comment at: clang/lib/Sema/SemaStmt.cpp:659
+                                       bool IsCallee) -> bool {
+    if (isa<CXXConstructorDecl>(CMD) || isa<CXXDestructorDecl>(CMD)) {
+      Diag(St->getBeginLoc(), diag::err_musttail_structors_forbidden) << &MTA;
----------------



================
Comment at: clang/lib/Sema/SemaStmt.cpp:682
+    return false;
+  } else if (const CXXMethodDecl *CMD = dyn_cast<CXXMethodDecl>(CurContext)) {
+    // Caller is a class/struct method.
----------------



================
Comment at: clang/lib/Sema/SemaStmt.cpp:700
+    // Call is: obj->*method_ptr or obj.*method_ptr
+    const MemberPointerType *MPT = VD->getType()->castAs<MemberPointerType>();
+    CalleeType.This = QualType(MPT->getClass(), 0);
----------------
It'd be better not to go through the cast machinery twice -- you cast to the `MemberPointerType` and then cast to the same thing again (but in a different way).


================
Comment at: clang/lib/Sema/SemaStmtAttr.cpp:214
+                                SourceRange Range) {
+  MustTailAttr MTA(S.Context, A);
+
----------------
This can be removed entirely.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99517