[PATCH] D67740: [Consumed] Refactor and improve diagnostics

Thu Sep 19 16:34:46 PDT 2019

comex marked 4 inline comments as done.
comex added a comment.

In D67740#1675564 <https://reviews.llvm.org/D67740#1675564>, @dblaikie wrote:

> Looks like this might benefit from being split into independent changes - the work related to templates (I haven't looked closely, but I assume that's fairly indivisible) and the work related to other diagnostics seems fairly separable - and maybe there's other pieces too.

Okay, I'll take the set_typestate/test_typestate part out of this and submit it separately.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:3234
+  "consumed analysis attribute is attached to a "
+  "%select{static method|constructor}0 of class '%1'">,
+  InGroup<Consumed>, DefaultIgnore;
----------------
dblaikie wrote:
> "member function" would be more correct than "method" here (the diagnostics in this file using the word "method" are mostly for Objective C things)
> 
> Are there other diagnostics that are similar? (like maybe function "const" - which can't be on non-member, static member, or ctors - wouldn't that be the same here? Where's the failure path for a non-member (free) function? Could it be unified with the static member/ctor case you're adding?)
> 
> Ah, looks like in Attr.td these attributes could be flagged "NonStaticCXXMethod" rather than "CXXMethod" - perhaps that narrower classification wasn't available when this was originally implemented. (then, I think, the attribute parsing logic will do the job of warning about the misuse and ignoring the attribute entirely)
That doesn't check for constructors, but sure, I'll add a "NonStaticNonConstructorCXXMethod" instead (in the separate submission).

================
Comment at: lib/Sema/SemaDeclAttr.cpp:1237-1240
+    else if (auto *CX = dyn_cast<CXXConstructorDecl>(X))
+      return CX->getThisType()->getPointeeType();
+    else
+      return cast<FunctionDecl>(X)->getCallResultType();
----------------
dblaikie wrote:
> Drop the else-after-return https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return
Done.

Repository:
  rC Clang

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

https://reviews.llvm.org/D67740