[cfe-commits] PATCH: Adds -Wself-assign to Clang, but defaults it to off

[Douglas Gregor]

On Dec 29, 2010, at 2:05 PM, Chandler Carruth wrote:

> This adds a warning to Clang for self-assignment of the form:
> 
> int a = 42;
> a = a;
> 
> Code with this often is merely trying to mark a variable used, but there is a more idiomatic way of doing that:
> 
> (void)a;
> 
> The warning is mostly a stylistic issue to help catch unintended self-assignments which result from typos in a codebase which strictly enforces that it shouldn't be used intentionally. As such, it is ignored by default, but can be turned on with -Wself-assign.

--- a/include/clang/Basic/DiagnosticGroups.td
+++ b/include/clang/Basic/DiagnosticGroups.td
@@ -95,6 +95,7 @@ def : DiagGroup<"pointer-to-int-cast">;
 def : DiagGroup<"redundant-decls">;
 def ReturnType : DiagGroup<"return-type">;
 def BindToTemporaryCopy : DiagGroup<"bind-to-temporary-copy">;
+def SelfAssignment : DiagGroup<"self-assign">;
 def SemiBeforeMethodBody : DiagGroup<"semicolon-before-method-body">;
 def : DiagGroup<"sequence-point">;
 def Shadow : DiagGroup<"shadow">;

Turn this on with -Wall, perhaps? I'd eventually like to have this on by default, but starting with -Wall seems best.

--- a/lib/Sema/SemaExpr.cpp
+++ b/lib/Sema/SemaExpr.cpp
@@ -7458,6 +7458,33 @@ static inline UnaryOperatorKind ConvertTokenKindToUnaryOpcode(
   return Opc;
 }
 
+/// DiagnoseSelfAssignment - Emits a warning if a value is assigned to itself.
+/// This warning is only emitted for builtin assgnment operations. It is also
+/// suppressed in the event of macro expansions.

Typo "assgnment".

+static void DiagnoseSelfAssignment(Sema &S, Expr *lhs, Expr *rhs,
+                                   SourceLocation OpLoc) {
+  if (!S.ActiveTemplateInstantiations.empty())
+    return;
+  if (OpLoc.isInvalid() || OpLoc.isMacroID())
+    return;
+  lhs = lhs->IgnoreParenImpCasts();
+  rhs = rhs->IgnoreParenImpCasts();
+  const DeclRefExpr *LeftDeclRef = dyn_cast<DeclRefExpr>(lhs);
+  const DeclRefExpr *RightDeclRef = dyn_cast<DeclRefExpr>(rhs);
+  if (!LeftDeclRef || !RightDeclRef ||
+      LeftDeclRef->getLocation().isMacroID() ||
+      RightDeclRef->getLocation().isMacroID())
+    return;
+  const Decl *LeftDecl = LeftDeclRef->getDecl()->getCanonicalDecl();
+  const Decl *RightDecl = RightDeclRef->getDecl()->getCanonicalDecl();
+  if (LeftDecl != RightDecl)
+    return;

Hrm, should you check whether the type of the declaration is a volatile (or a reference to volatile)?


+  S.Diag(OpLoc, diag::warn_self_assignment)
+      << LeftDeclRef->getType()
+      << lhs->getSourceRange() << rhs->getSourceRange();
+}
+

It would be really nice if we could follow up this warning with a note + Fix-It, e.g.,

  note: cast 'thevariablename' to 'void' to silence this warning

with a Fix-It that replaces the expression with

  (void)thevariablename

Perhaps we could add this note as a follow-on when an expression statement consists of just a self-assignment?

	- Doug


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110103/08fa35b6/attachment.html>