[PATCH] D146323: inline stmt attribute diagnosing in templates

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/docs/ReleaseNotes.rst:179
   by prioritizing ``-Wunreachable-code-fallthrough``.
+- Clang now correctly diagnoses statement attributes ``[[clang::always_inine]]` and
+  ``[[clang::noinline]]`` when used on a statement with dependent call expressions.
----------------



================
Comment at: clang/lib/Sema/SemaStmtAttr.cpp:225-231
+  // If the call expressions lists are equal in size, we can  skip
+  // previously emitted diagnostics.  However, if the statement has a pack
+  // expansion, we have no way of telling which CallExpr is the instantiated
+  // version of the other.  In this case, we will end up re-diagnosing in the
+  // instantiation.
+  // ie: [[clang::always_inline]] non_dependent(), (other_call<Pack>()...)
+  // will diagnose non_dependent again.
----------------
Minor cleanup; NFC


================
Comment at: clang/lib/Sema/SemaStmtAttr.cpp:241
+
+  for (unsigned I = 0; I < CEF.getCallExprs().size(); ++I) {
+    // If the original call expression already had a callee, we already
----------------
Range-based for loop using `llvm::enumerate()` so you can keep the index?


================
Comment at: clang/lib/Sema/TreeTransform.h:411-412
+#define ATTR(X)                                                                \
+  const X##Attr *TransformStmt##X##Attr(const Stmt *OrigS, const Stmt *InstS,  \
+                                        const X##Attr *A) {                    \
+    return getDerived().Transform##X##Attr(A);                                 \
----------------
So we don't get unused parameter warnings.


================
Comment at: clang/lib/Sema/TreeTransform.h:7617
   for (const auto *I : S->getAttrs()) {
-    const Attr *R = getDerived().TransformAttr(I);
+    const Attr *R = getDerived().TransformStmtAttr(S, SubStmt.get(), I);
     AttrsChanged |= (I != R);
----------------
Hmmm, this seems a bit inconsistent -- why do we pass the `AttributedStmt` as the first argument and not `S->getSubStmt()`? It may be totally reasonable, but should we make the first parameter of `TransformStmtAttr()` be `const AttributedStmt *` instead of `const Stmt *` so it's clear to callers that what they're getting for the original is already wrapped as opposed to unwrapped like the second argument?


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

https://reviews.llvm.org/D146323