[cfe-commits] PATCH: Adds -Wself-assign to Clang, but defaults it to off
Douglas Gregor
dgregor at apple.com
Mon Jan 3 18:28:10 PST 2011
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>
More information about the cfe-commits
mailing list