[PATCH] D26911: readability-redundant-declarations: Fix crash

[Daniel Marjamäki via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL287540: readability-redundant-declaration: Fix crash 
(authored by danielmarjamaki).

Changed prior to commit:
  https://reviews.llvm.org/D26911?vs=78706=78715#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D26911

Files:
  clang-tools-extra/trunk/clang-tidy/readability/RedundantDeclarationCheck.cpp
  clang-tools-extra/trunk/test/clang-tidy/readability-redundant-declaration.cpp


Index: 
clang-tools-extra/trunk/clang-tidy/readability/RedundantDeclarationCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/readability/RedundantDeclarationCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/readability/RedundantDeclarationCheck.cpp
@@ -28,6 +28,8 @@
   const auto *Prev = D->getPreviousDecl();
   if (!Prev)
 return;
+  if (!Prev->getLocation().isValid())
+return;
   if (Prev->getLocation() == D->getLocation())
 return;
 
Index: 
clang-tools-extra/trunk/test/clang-tidy/readability-redundant-declaration.cpp
===
--- 
clang-tools-extra/trunk/test/clang-tidy/readability-redundant-declaration.cpp
+++ 
clang-tools-extra/trunk/test/clang-tidy/readability-redundant-declaration.cpp
@@ -21,3 +21,10 @@
 // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: redundant 'f' declaration
 // CHECK-FIXES: {{^}}{{$}}
 static int f() {}
+
+// Original check crashed for the code below.
+namespace std {
+  typedef long unsigned int size_t;
+}
+void* operator new(std::size_t) __attribute__((__externally_visible__));
+void* operator new[](std::size_t) __attribute__((__externally_visible__));


Index: clang-tools-extra/trunk/clang-tidy/readability/RedundantDeclarationCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/readability/RedundantDeclarationCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/readability/RedundantDeclarationCheck.cpp
@@ -28,6 +28,8 @@
   const auto *Prev = D->getPreviousDecl();
   if (!Prev)
 return;
+  if (!Prev->getLocation().isValid())
+return;
   if (Prev->getLocation() == D->getLocation())
 return;
 
Index: clang-tools-extra/trunk/test/clang-tidy/readability-redundant-declaration.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/readability-redundant-declaration.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/readability-redundant-declaration.cpp
@@ -21,3 +21,10 @@
 // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: redundant 'f' declaration
 // CHECK-FIXES: {{^}}{{$}}
 static int f() {}
+
+// Original check crashed for the code below.
+namespace std {
+  typedef long unsigned int size_t;
+}
+void* operator new(std::size_t) __attribute__((__externally_visible__));
+void* operator new[](std::size_t) __attribute__((__externally_visible__));
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D24656: [clang-tidy] Add check readability-redundant-declaration

[Daniel Marjamäki via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL285689: [clang-tidy] Add check 
readability-redundant-declaration (authored by danielmarjamaki).

Changed prior to commit:
  https://reviews.llvm.org/D24656?vs=74478=76548#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D24656

Files:
  clang-tools-extra/trunk/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/trunk/clang-tidy/readability/RedundantDeclarationCheck.cpp
  clang-tools-extra/trunk/clang-tidy/readability/RedundantDeclarationCheck.h
  clang-tools-extra/trunk/docs/ReleaseNotes.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/trunk/docs/clang-tidy/checks/readability-redundant-declaration.rst
  clang-tools-extra/trunk/test/clang-tidy/readability-redundant-declaration.cpp

Index: clang-tools-extra/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp
===
--- clang-tools-extra/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp
+++ clang-tools-extra/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -23,6 +23,7 @@
 #include "NamedParameterCheck.h"
 #include "NonConstParameterCheck.h"
 #include "RedundantControlFlowCheck.h"
+#include "RedundantDeclarationCheck.h"
 #include "RedundantMemberInitCheck.h"
 #include "RedundantSmartptrGetCheck.h"
 #include "RedundantStringCStrCheck.h"
@@ -68,6 +69,8 @@
 "readability-non-const-parameter");
 CheckFactories.registerCheck(
 "readability-redundant-control-flow");
+CheckFactories.registerCheck(
+"readability-redundant-declaration");
 CheckFactories.registerCheck(
 "readability-redundant-smartptr-get");
 CheckFactories.registerCheck(
Index: clang-tools-extra/trunk/clang-tidy/readability/CMakeLists.txt
===
--- clang-tools-extra/trunk/clang-tidy/readability/CMakeLists.txt
+++ clang-tools-extra/trunk/clang-tidy/readability/CMakeLists.txt
@@ -16,6 +16,7 @@
   NonConstParameterCheck.cpp
   ReadabilityTidyModule.cpp
   RedundantControlFlowCheck.cpp
+  RedundantDeclarationCheck.cpp
   RedundantMemberInitCheck.cpp
   RedundantStringCStrCheck.cpp
   RedundantSmartptrGetCheck.cpp
Index: clang-tools-extra/trunk/clang-tidy/readability/RedundantDeclarationCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/readability/RedundantDeclarationCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/readability/RedundantDeclarationCheck.cpp
@@ -0,0 +1,71 @@
+//===--- RedundantDeclarationCheck.cpp - clang-tidy===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "RedundantDeclarationCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+void RedundantDeclarationCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(namedDecl(anyOf(varDecl(), functionDecl())).bind("Decl"), this);
+}
+
+void RedundantDeclarationCheck::check(const MatchFinder::MatchResult ) {
+  const NamedDecl *D = Result.Nodes.getNodeAs("Decl");
+  const auto *Prev = D->getPreviousDecl();
+  if (!Prev)
+return;
+  if (Prev->getLocation() == D->getLocation())
+return;
+
+  const SourceManager  = *Result.SourceManager;
+
+  const bool DifferentHeaders =
+   !SM.isInMainFile(D->getLocation()) &&
+   !SM.isWrittenInSameFile(Prev->getLocation(), D->getLocation());
+
+  bool MultiVar = false;
+  if (const auto *VD = dyn_cast(D)) {
+if (VD->getPreviousDecl()->getStorageClass() == SC_Extern &&
+VD->getStorageClass() != SC_Extern)
+  return;
+// Is this a multivariable declaration?
+for (const auto Other : VD->getDeclContext()->decls()) {
+  if (Other != D && Other->getLocStart() == VD->getLocStart()) {
+MultiVar = true;
+break;
+  }
+}
+  } else {
+const auto *FD = cast(D);
+if (FD->isThisDeclarationADefinition())
+  return;
+  }
+
+  SourceLocation EndLoc = Lexer::getLocForEndOfToken(
+  D->getSourceRange().getEnd(), 0, SM, Result.Context->getLangOpts());
+  {
+auto Diag = diag(D->getLocation(), "redundant %0 declaration")
+<< D;
+if (!MultiVar && !DifferentHeaders)
+  Diag << FixItHint::CreateRemoval(
+  SourceRange(D->getSourceRange().getBegin(), EndLoc));
+  }
+  diag(Prev->getLocation(), "previously declared here", DiagnosticIDs::Note);
+}
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
Index: 

[PATCH] D25606: alpha.core.UnreachableCode - don't warn about unreachable code inside macro

[Daniel Marjamäki via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL284477: alpha.core.UnreachableCode - don't warn about 
unreachable code inside macro (authored by danielmarjamaki).

Changed prior to commit:
  https://reviews.llvm.org/D25606?vs=74849=74990#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D25606

Files:
  cfe/trunk/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
  cfe/trunk/test/Analysis/unreachable-code-path.c


Index: cfe/trunk/test/Analysis/unreachable-code-path.c
===
--- cfe/trunk/test/Analysis/unreachable-code-path.c
+++ cfe/trunk/test/Analysis/unreachable-code-path.c
@@ -206,3 +206,10 @@
   int x = inlineFunction(i);
   x && x < 10; // no-warning
 }
+
+// Don't warn in a macro
+#define RETURN(X)  do { return; } while (0)
+void macro(void) {
+  RETURN(1); // no-warning
+}
+
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
@@ -147,6 +147,14 @@
 PathDiagnosticLocation DL;
 SourceLocation SL;
 if (const Stmt *S = getUnreachableStmt(CB)) {
+  // In macros, 'do {...} while (0)' is often used. Don't warn about the
+  // condition 0 when it is unreachable.
+  if (S->getLocStart().isMacroID())
+if (const auto *I = dyn_cast(S))
+  if (I->getValue() == 0ULL)
+if (const Stmt *Parent = PM->getParent(S))
+  if (isa(Parent))
+continue;
   SR = S->getSourceRange();
   DL = PathDiagnosticLocation::createBegin(S, B.getSourceManager(), LC);
   SL = DL.asLocation();


Index: cfe/trunk/test/Analysis/unreachable-code-path.c
===
--- cfe/trunk/test/Analysis/unreachable-code-path.c
+++ cfe/trunk/test/Analysis/unreachable-code-path.c
@@ -206,3 +206,10 @@
   int x = inlineFunction(i);
   x && x < 10; // no-warning
 }
+
+// Don't warn in a macro
+#define RETURN(X)  do { return; } while (0)
+void macro(void) {
+  RETURN(1); // no-warning
+}
+
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
@@ -147,6 +147,14 @@
 PathDiagnosticLocation DL;
 SourceLocation SL;
 if (const Stmt *S = getUnreachableStmt(CB)) {
+  // In macros, 'do {...} while (0)' is often used. Don't warn about the
+  // condition 0 when it is unreachable.
+  if (S->getLocStart().isMacroID())
+if (const auto *I = dyn_cast(S))
+  if (I->getValue() == 0ULL)
+if (const Stmt *Parent = PM->getParent(S))
+  if (isa(Parent))
+continue;
   SR = S->getSourceRange();
   DL = PathDiagnosticLocation::createBegin(S, B.getSourceManager(), LC);
   SL = DL.asLocation();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D24656: [clang-tidy] Add check readability-redundant-declaration

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a comment.

Ping.


Repository:
  rL LLVM

https://reviews.llvm.org/D24656



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D25606: alpha.core.UnreachableCode - don't warn about unreachable code inside macro

[Daniel Marjamäki via cfe-commits]

danielmarjamaki updated this revision to Diff 74849.
danielmarjamaki added a comment.

make pattern to avoid warnings more specific


Repository:
  rL LLVM

https://reviews.llvm.org/D25606

Files:
  lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
  test/Analysis/unreachable-code-path.c


Index: test/Analysis/unreachable-code-path.c
===
--- test/Analysis/unreachable-code-path.c
+++ test/Analysis/unreachable-code-path.c
@@ -206,3 +206,10 @@
   int x = inlineFunction(i);
   x && x < 10; // no-warning
 }
+
+// Don't warn in a macro
+#define RETURN(X)  do { return; } while (0)
+void macro(void) {
+  RETURN(1); // no-warning
+}
+
Index: lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
@@ -147,6 +147,14 @@
 PathDiagnosticLocation DL;
 SourceLocation SL;
 if (const Stmt *S = getUnreachableStmt(CB)) {
+  // In macros, 'do {...} while (0)' is often used. Don't warn about the
+  // condition 0 when it is unreachable.
+  if (S->getLocStart().isMacroID())
+if (const auto *I = dyn_cast(S))
+  if (I->getValue() == 0ULL)
+if (const Stmt *Parent = PM->getParent(S))
+  if (isa(Parent))
+continue;
   SR = S->getSourceRange();
   DL = PathDiagnosticLocation::createBegin(S, B.getSourceManager(), LC);
   SL = DL.asLocation();


Index: test/Analysis/unreachable-code-path.c
===
--- test/Analysis/unreachable-code-path.c
+++ test/Analysis/unreachable-code-path.c
@@ -206,3 +206,10 @@
   int x = inlineFunction(i);
   x && x < 10; // no-warning
 }
+
+// Don't warn in a macro
+#define RETURN(X)  do { return; } while (0)
+void macro(void) {
+  RETURN(1); // no-warning
+}
+
Index: lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
@@ -147,6 +147,14 @@
 PathDiagnosticLocation DL;
 SourceLocation SL;
 if (const Stmt *S = getUnreachableStmt(CB)) {
+  // In macros, 'do {...} while (0)' is often used. Don't warn about the
+  // condition 0 when it is unreachable.
+  if (S->getLocStart().isMacroID())
+if (const auto *I = dyn_cast(S))
+  if (I->getValue() == 0ULL)
+if (const Stmt *Parent = PM->getParent(S))
+  if (isa(Parent))
+continue;
   SR = S->getSourceRange();
   DL = PathDiagnosticLocation::createBegin(S, B.getSourceManager(), LC);
   SL = DL.asLocation();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D25606: alpha.core.UnreachableCode - don't warn about unreachable code inside macro

[Daniel Marjamäki via cfe-commits]

danielmarjamaki created this revision.
danielmarjamaki added reviewers: NoQ, dcoughlin.
danielmarjamaki added subscribers: cfe-commits, xazax.hun, zaks.anna, a.sidorin.
danielmarjamaki set the repository for this revision to rL LLVM.

This patch fixes false positives for such code:

  #define RETURN(X)  do { return; } while (0)
  
  void dostuff(void) {
RETURN(1); // no-warning
  }

The condition "0" in the macro is unreachable but that condition is there for a 
good reason.


Repository:
  rL LLVM

https://reviews.llvm.org/D25606

Files:
  lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
  test/Analysis/unreachable-code-path.c


Index: test/Analysis/unreachable-code-path.c
===
--- test/Analysis/unreachable-code-path.c
+++ test/Analysis/unreachable-code-path.c
@@ -206,3 +206,10 @@
   int x = inlineFunction(i);
   x && x < 10; // no-warning
 }
+
+// Don't warn in a macro
+#define RETURN(X)  do { return; } while (0)
+void macro(void) {
+  RETURN(1); // no-warning
+}
+
Index: lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
@@ -147,6 +147,8 @@
 PathDiagnosticLocation DL;
 SourceLocation SL;
 if (const Stmt *S = getUnreachableStmt(CB)) {
+  if (S->getLocStart().isMacroID())
+continue;
   SR = S->getSourceRange();
   DL = PathDiagnosticLocation::createBegin(S, B.getSourceManager(), LC);
   SL = DL.asLocation();


Index: test/Analysis/unreachable-code-path.c
===
--- test/Analysis/unreachable-code-path.c
+++ test/Analysis/unreachable-code-path.c
@@ -206,3 +206,10 @@
   int x = inlineFunction(i);
   x && x < 10; // no-warning
 }
+
+// Don't warn in a macro
+#define RETURN(X)  do { return; } while (0)
+void macro(void) {
+  RETURN(1); // no-warning
+}
+
Index: lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
@@ -147,6 +147,8 @@
 PathDiagnosticLocation DL;
 SourceLocation SL;
 if (const Stmt *S = getUnreachableStmt(CB)) {
+  if (S->getLocStart().isMacroID())
+continue;
   SR = S->getSourceRange();
   DL = PathDiagnosticLocation::createBegin(S, B.getSourceManager(), LC);
   SL = DL.asLocation();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D25596: alpha.core.Conversion - Fix false positive for 'U32 += S16; ' expression, that is not unsafe

[Daniel Marjamäki via cfe-commits]

danielmarjamaki created this revision.
danielmarjamaki added a reviewer: NoQ.
danielmarjamaki added subscribers: cfe-commits, xazax.hun, dcoughlin.
danielmarjamaki set the repository for this revision to rL LLVM.

This patch fix false positives for loss of sign in addition and subtraction 
assignment operators.


Repository:
  rL LLVM

https://reviews.llvm.org/D25596

Files:
  lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
  test/Analysis/conversion.c


Index: test/Analysis/conversion.c
===
--- test/Analysis/conversion.c
+++ test/Analysis/conversion.c
@@ -77,6 +77,10 @@
 void dontwarn5() {
   signed S = -32;
   U8 = S + 10;
+
+  unsigned  x = 100;
+  signed short delta = -1;
+  x += delta;
 }
 
 
Index: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
+++ lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
@@ -73,10 +73,13 @@
   // Loss of sign/precision in binary operation.
   if (const auto *B = dyn_cast(Parent)) {
 BinaryOperator::Opcode Opc = B->getOpcode();
-if (Opc == BO_Assign || Opc == BO_AddAssign || Opc == BO_SubAssign ||
-Opc == BO_MulAssign) {
-  LossOfSign = isLossOfSign(Cast, C);
-  LossOfPrecision = isLossOfPrecision(Cast, C);
+if (B->isAssignmentOp()) {
+  if (Opc == BO_Assign || Opc == BO_AddAssign || Opc == BO_SubAssign ||
+  Opc == BO_MulAssign || Opc == BO_OrAssign || Opc == BO_XorAssign)
+LossOfPrecision = isLossOfPrecision(Cast, C);
+  if (Opc == BO_Assign || Opc == BO_MulAssign || Opc == BO_DivAssign ||
+  Opc == BO_RemAssign)
+LossOfSign = isLossOfSign(Cast, C);
 } else if (B->isRelationalOp() || B->isMultiplicativeOp()) {
   LossOfSign = isLossOfSign(Cast, C);
 }
@@ -153,7 +156,7 @@
 }
 
 bool ConversionChecker::isLossOfPrecision(const ImplicitCastExpr *Cast,
-CheckerContext ) const {
+  CheckerContext ) const {
   // Don't warn about explicit loss of precision.
   if (Cast->isEvaluatable(C.getASTContext()))
 return false;
@@ -177,7 +180,7 @@
 }
 
 bool ConversionChecker::isLossOfSign(const ImplicitCastExpr *Cast,
-   CheckerContext ) const {
+ CheckerContext ) const {
   QualType CastType = Cast->getType();
   QualType SubType = Cast->IgnoreParenImpCasts()->getType();
 


Index: test/Analysis/conversion.c
===
--- test/Analysis/conversion.c
+++ test/Analysis/conversion.c
@@ -77,6 +77,10 @@
 void dontwarn5() {
   signed S = -32;
   U8 = S + 10;
+
+  unsigned  x = 100;
+  signed short delta = -1;
+  x += delta;
 }
 
 
Index: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
+++ lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
@@ -73,10 +73,13 @@
   // Loss of sign/precision in binary operation.
   if (const auto *B = dyn_cast(Parent)) {
 BinaryOperator::Opcode Opc = B->getOpcode();
-if (Opc == BO_Assign || Opc == BO_AddAssign || Opc == BO_SubAssign ||
-Opc == BO_MulAssign) {
-  LossOfSign = isLossOfSign(Cast, C);
-  LossOfPrecision = isLossOfPrecision(Cast, C);
+if (B->isAssignmentOp()) {
+  if (Opc == BO_Assign || Opc == BO_AddAssign || Opc == BO_SubAssign ||
+  Opc == BO_MulAssign || Opc == BO_OrAssign || Opc == BO_XorAssign)
+LossOfPrecision = isLossOfPrecision(Cast, C);
+  if (Opc == BO_Assign || Opc == BO_MulAssign || Opc == BO_DivAssign ||
+  Opc == BO_RemAssign)
+LossOfSign = isLossOfSign(Cast, C);
 } else if (B->isRelationalOp() || B->isMultiplicativeOp()) {
   LossOfSign = isLossOfSign(Cast, C);
 }
@@ -153,7 +156,7 @@
 }
 
 bool ConversionChecker::isLossOfPrecision(const ImplicitCastExpr *Cast,
-CheckerContext ) const {
+  CheckerContext ) const {
   // Don't warn about explicit loss of precision.
   if (Cast->isEvaluatable(C.getASTContext()))
 return false;
@@ -177,7 +180,7 @@
 }
 
 bool ConversionChecker::isLossOfSign(const ImplicitCastExpr *Cast,
-   CheckerContext ) const {
+ CheckerContext ) const {
   QualType CastType = Cast->getType();
   QualType SubType = Cast->IgnoreParenImpCasts()->getType();
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D25326: [StaticAnalyser] Don't merge different returns in ExplodedGraph

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a comment.

I agree with the comments from you dcoughlin but I am not sure how to do it.

> Can you also add a test that tests this more directly (i.e., with 
> clang_analyzer_warnIfReached). I don't think it is good to have the only test 
> for this core coverage issue to be in tests for an alpha checker. Adding the 
> direct test would also make it easier to track down any regression if it 
> happens. The 'func.c' test file might be a good place for such a test.

I totally agree.

In func.c there such comments:
// expected-warning{{FALSE|TRUE|UNKNOWN}}

what does those FALSE|TRUE|UNKNOWN do?

I don't see what this will do:

  clang_analyzer_eval(!f);

I want that both returns are reached. and I want to ensure that result from 
function is both 1 and 0.

> You could also try to add a canary with clang analyzer eval after the if 
> statement to force the test to fail if we do add this symbolic reasoning.

sounds good. sorry but I don't see how to do it.


Repository:
  rL LLVM

https://reviews.llvm.org/D25326



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D24861: [Sema] extend Wshift-op-parentheses so it warns for multiplicative operators

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a comment.

Ping


Repository:
  rL LLVM

https://reviews.llvm.org/D24861



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D24656: [clang-tidy] Add check readability-redundant-declaration

[Daniel Marjamäki via cfe-commits]

danielmarjamaki set the repository for this revision to rL LLVM.
danielmarjamaki updated this revision to Diff 74478.
danielmarjamaki added a comment.
Herald added a subscriber: modocache.

changed cast(D)->getName() to cast(D)


Repository:
  rL LLVM

https://reviews.llvm.org/D24656

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tidy/readability/RedundantDeclarationCheck.cpp
  clang-tidy/readability/RedundantDeclarationCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-redundant-declaration.rst
  test/clang-tidy/readability-redundant-declaration.cpp

Index: test/clang-tidy/readability-redundant-declaration.cpp
===
--- test/clang-tidy/readability-redundant-declaration.cpp
+++ test/clang-tidy/readability-redundant-declaration.cpp
@@ -0,0 +1,23 @@
+// RUN: %check_clang_tidy %s readability-redundant-declaration %t
+
+extern int Xyz;
+extern int Xyz;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: redundant 'Xyz' declaration [readability-redundant-declaration]
+// CHECK-FIXES: {{^}}{{$}}
+int Xyz = 123;
+
+extern int A;
+extern int A, B;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: redundant 'A' declaration
+// CHECK-FIXES: {{^}}extern int A, B;{{$}}
+
+extern int Buf[10];
+extern int Buf[10];
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: redundant 'Buf' declaration
+// CHECK-FIXES: {{^}}{{$}}
+
+static int f();
+static int f();
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: redundant 'f' declaration
+// CHECK-FIXES: {{^}}{{$}}
+static int f() {}
Index: docs/clang-tidy/checks/readability-redundant-declaration.rst
===
--- docs/clang-tidy/checks/readability-redundant-declaration.rst
+++ docs/clang-tidy/checks/readability-redundant-declaration.rst
@@ -0,0 +1,17 @@
+.. title:: clang-tidy - readability-redundant-declaration
+
+readability-redundant-declaration
+=
+
+Finds redundant variable declarations.
+
+.. code-block:: c++
+
+  extern int X;
+  extern int X;
+
+becomes
+
+.. code-block:: c++
+
+  extern int X;
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -133,6 +133,7 @@
readability-named-parameter
readability-non-const-parameter
readability-redundant-control-flow
+   readability-redundant-declaration
readability-redundant-smartptr-get
readability-redundant-string-cstr
readability-redundant-string-init
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -111,6 +111,11 @@
   Flags function parameters of a pointer type that could be changed to point to
   a constant type instead.
 
+- New `readability-redundant-declaration
+  `_ check
+
+  Warns about duplicate variable declarations.
+
 Fixed bugs:
 
 - `modernize-make-unique
Index: clang-tidy/readability/RedundantDeclarationCheck.h
===
--- clang-tidy/readability/RedundantDeclarationCheck.h
+++ clang-tidy/readability/RedundantDeclarationCheck.h
@@ -0,0 +1,35 @@
+//===--- RedundantDeclarationCheck.h - clang-tidy*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANT_DECLARATION_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANT_DECLARATION_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Find redundant variable declarations.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-redundant-declaration.html
+class RedundantDeclarationCheck : public ClangTidyCheck {
+public:
+  RedundantDeclarationCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult ) override;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANT_DECLARATION_H
Index: clang-tidy/readability/RedundantDeclarationCheck.cpp
===
--- clang-tidy/readability/RedundantDeclarationCheck.cpp
+++ clang-tidy/readability/RedundantDeclarationCheck.cpp
@@ -0,0 +1,71 @@
+//===--- 

[PATCH] D24656: [clang-tidy] Add check readability-redundant-declaration

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added inline comments.



Comment at: clang-tidy/readability/RedundantDeclarationCheck.cpp:60
+auto Diag = diag(D->getLocation(), "redundant '%0' declaration")
+<< cast(D)->getName();
+if (!MultiVar && !DifferentHeaders)

alexfh wrote:
> It should be possible to just use `D` here.
Thanks. It's not possible:

```
RedundantDeclarationCheck.cpp:61:15: error: ‘const class clang::Decl’ has no 
member named ‘getName’
 << D->getName();
   ^
```



https://reviews.llvm.org/D24656



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D25326: [StaticAnalyser] Don't merge different returns in ExplodedGraph

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a comment.

In https://reviews.llvm.org/D25326#564584, @zaks.anna wrote:

> Please, fix the style issues before committing.


Sorry I missed that.

Ideally it would be possible to run clang-format on the files before 
committing. but currently I get lots of unrelated changes then.

Would it be ok to run clang-format on some files to clean up the formatting? At 
least some minor changes like removing trailing spaces.


Repository:
  rL LLVM

https://reviews.llvm.org/D25326



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D25326: [StaticAnalyser] Don't merge different returns in ExplodedGraph

[Daniel Marjamäki via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL283554: [analyzer] Don't merge different return nodes in 
ExplodedGraph (authored by danielmarjamaki).

Changed prior to commit:
  https://reviews.llvm.org/D25326?vs=73926=73947#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D25326

Files:
  cfe/trunk/include/clang/Analysis/ProgramPoint.h
  cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
  cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SubEngine.h
  cfe/trunk/lib/StaticAnalyzer/Core/CoreEngine.cpp
  cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp
  cfe/trunk/test/Analysis/inlining/InlineObjCClassMethod.m
  cfe/trunk/test/Analysis/unreachable-code-path.c

Index: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SubEngine.h
===
--- cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SubEngine.h
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SubEngine.h
@@ -109,7 +109,8 @@
   /// Called by CoreEngine.  Used to notify checkers that processing a
   /// function has ended. Called for both inlined and and top-level functions.
   virtual void processEndOfFunction(NodeBuilderContext& BC,
-ExplodedNode *Pred) = 0;
+ExplodedNode *Pred,
+const ReturnStmt *RS = nullptr) = 0;
 
   // Generate the entry node of the callee.
   virtual void processCallEnter(NodeBuilderContext& BC, CallEnter CE,
Index: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
===
--- cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -262,7 +262,8 @@
   /// Called by CoreEngine.  Used to notify checkers that processing a
   /// function has ended. Called for both inlined and and top-level functions.
   void processEndOfFunction(NodeBuilderContext& BC,
-ExplodedNode *Pred) override;
+ExplodedNode *Pred,
+const ReturnStmt *RS=nullptr) override;
 
   /// Remove dead bindings/symbols before exiting a function.
   void removeDeadOnEndOfFunction(NodeBuilderContext& BC,
Index: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
===
--- cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
@@ -109,7 +109,8 @@
   CoreEngine(const CoreEngine &) = delete;
   void operator=(const CoreEngine &) = delete;
 
-  ExplodedNode *generateCallExitBeginNode(ExplodedNode *N);
+  ExplodedNode *generateCallExitBeginNode(ExplodedNode *N,
+  const ReturnStmt *RS);
 
 public:
   /// Construct a CoreEngine object to analyze the provided CFG.
@@ -172,7 +173,7 @@
 
   /// \brief enqueue the nodes corresponding to the end of function onto the
   /// end of path / work list.
-  void enqueueEndOfFunction(ExplodedNodeSet );
+  void enqueueEndOfFunction(ExplodedNodeSet , const ReturnStmt *RS);
 
   /// \brief Enqueue a single node created as a result of statement processing.
   void enqueueStmtNode(ExplodedNode *N, const CFGBlock *Block, unsigned Idx);
Index: cfe/trunk/include/clang/Analysis/ProgramPoint.h
===
--- cfe/trunk/include/clang/Analysis/ProgramPoint.h
+++ cfe/trunk/include/clang/Analysis/ProgramPoint.h
@@ -622,8 +622,8 @@
 class CallExitBegin : public ProgramPoint {
 public:
   // CallExitBegin uses the callee's location context.
-  CallExitBegin(const StackFrameContext *L)
-: ProgramPoint(nullptr, CallExitBeginKind, L, nullptr) {}
+  CallExitBegin(const StackFrameContext *L, const ReturnStmt *RS)
+: ProgramPoint(RS, CallExitBeginKind, L, nullptr) { }
 
 private:
   friend class ProgramPoint;
Index: cfe/trunk/test/Analysis/unreachable-code-path.c
===
--- cfe/trunk/test/Analysis/unreachable-code-path.c
+++ cfe/trunk/test/Analysis/unreachable-code-path.c
@@ -194,3 +194,15 @@
 break;
   }
 }
+
+// Don't merge return nodes in ExplodedGraph unless they are same.
+extern int table[];
+static int inlineFunction(const int i) {
+  if (table[i] != 0)
+return 1;
+  return 0;
+}
+void test13(int i) {
+  int x = inlineFunction(i);
+  x && x < 10; // no-warning
+}
Index: cfe/trunk/test/Analysis/inlining/InlineObjCClassMethod.m
===
--- cfe/trunk/test/Analysis/inlining/InlineObjCClassMethod.m
+++ cfe/trunk/test/Analysis/inlining/InlineObjCClassMethod.m
@@ 

[PATCH] D25326: [StaticAnalyser] Don't merge different returns in ExplodedGraph

[Daniel Marjamäki via cfe-commits]

danielmarjamaki removed rL LLVM as the repository for this revision.
danielmarjamaki updated this revision to Diff 73926.
danielmarjamaki added a comment.

Refactoring.


https://reviews.llvm.org/D25326

Files:
  include/clang/Analysis/ProgramPoint.h
  include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
  include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  include/clang/StaticAnalyzer/Core/PathSensitive/SubEngine.h
  lib/StaticAnalyzer/Core/CoreEngine.cpp
  lib/StaticAnalyzer/Core/ExprEngine.cpp
  test/Analysis/inlining/InlineObjCClassMethod.m
  test/Analysis/unreachable-code-path.c

Index: test/Analysis/unreachable-code-path.c
===
--- test/Analysis/unreachable-code-path.c
+++ test/Analysis/unreachable-code-path.c
@@ -194,3 +194,15 @@
 break;
   }
 }
+
+// Don't merge return nodes in ExplodedGraph unless they are same.
+extern int table[];
+static int inlineFunction(const int i) {
+  if (table[i] != 0)
+return 1;
+  return 0;
+}
+void test13(int i) {
+  int x = inlineFunction(i);
+  x && x < 10; // no-warning
+}
Index: test/Analysis/inlining/InlineObjCClassMethod.m
===
--- test/Analysis/inlining/InlineObjCClassMethod.m
+++ test/Analysis/inlining/InlineObjCClassMethod.m
@@ -174,12 +174,12 @@
 @implementation MyClassSelf
 + (int)testClassMethodByKnownVarDecl {
   int y = [MyParentSelf testSelf];
-  return 5/y; // Should warn here.
+  return 5/y; // expected-warning{{Division by zero}}
 }
 @end
 int foo2() {
   int y = [MyParentSelf testSelf];
-  return 5/y; // Should warn here.
+  return 5/y; // expected-warning{{Division by zero}}
 }
 
 // TODO: We do not inline 'getNum' in the following case, where the value of 
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -1766,7 +1766,8 @@
 /// ProcessEndPath - Called by CoreEngine.  Used to generate end-of-path
 ///  nodes when the control reaches the end of a function.
 void ExprEngine::processEndOfFunction(NodeBuilderContext& BC,
-  ExplodedNode *Pred) {
+  ExplodedNode *Pred,
+  const ReturnStmt *RS) {
   // FIXME: Assert that stackFrameDoesNotContainInitializedTemporaries(*Pred)).
   // We currently cannot enable this assert, as lifetime extended temporaries
   // are not modelled correctly.
@@ -1788,7 +1789,7 @@
 getCheckerManager().runCheckersForEndFunction(BC, Dst, Pred, *this);
   }
 
-  Engine.enqueueEndOfFunction(Dst);
+  Engine.enqueueEndOfFunction(Dst,RS);
 }
 
 /// ProcessSwitch - Called by CoreEngine.  Used to generate successor
Index: lib/StaticAnalyzer/Core/CoreEngine.cpp
===
--- lib/StaticAnalyzer/Core/CoreEngine.cpp
+++ lib/StaticAnalyzer/Core/CoreEngine.cpp
@@ -309,8 +309,19 @@
 assert (L.getLocationContext()->getCFG()->getExit().size() == 0
 && "EXIT block cannot contain Stmts.");
 
+// Get return statement..
+const ReturnStmt *RS = nullptr;
+if (!L.getSrc()->empty()) {
+  if (Optional LastStmt = L.getSrc()->back().getAs()) {
+if (RS = dyn_cast(LastStmt->getStmt())) {
+  if (!RS->getRetValue())
+RS = nullptr;
+}
+  }
+}
+
 // Process the final state transition.
-SubEng.processEndOfFunction(BuilderCtx, Pred);
+SubEng.processEndOfFunction(BuilderCtx, Pred, RS);
 
 // This path is done. Don't enqueue any more nodes.
 return;
@@ -589,13 +600,14 @@
 WList->enqueue(Succ, Block, Idx+1);
 }
 
-ExplodedNode *CoreEngine::generateCallExitBeginNode(ExplodedNode *N) {
+ExplodedNode *CoreEngine::generateCallExitBeginNode(ExplodedNode *N,
+const ReturnStmt *RS) {
   // Create a CallExitBegin node and enqueue it.
   const StackFrameContext *LocCtx
  = cast(N->getLocationContext());
 
   // Use the callee location context.
-  CallExitBegin Loc(LocCtx);
+  CallExitBegin Loc(LocCtx,RS);
 
   bool isNew;
   ExplodedNode *Node = G.getNode(Loc, N->getState(), false, );
@@ -619,12 +631,12 @@
   }
 }
 
-void CoreEngine::enqueueEndOfFunction(ExplodedNodeSet ) {
+void CoreEngine::enqueueEndOfFunction(ExplodedNodeSet , const ReturnStmt *RS) {
   for (ExplodedNodeSet::iterator I = Set.begin(), E = Set.end(); I != E; ++I) {
 ExplodedNode *N = *I;
 // If we are in an inlined call, generate CallExitBegin node.
 if (N->getLocationContext()->getParent()) {
-  N = generateCallExitBeginNode(N);
+  N = generateCallExitBeginNode(N,RS);
   if (N)
 WList->enqueue(N);
 } else {
Index: include/clang/StaticAnalyzer/Core/PathSensitive/SubEngine.h

[PATCH] D25326: [StaticAnalyser] Don't merge different returns in ExplodedGraph

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a comment.

In https://reviews.llvm.org/D25326#564291, @danielmarjamaki wrote:

> In https://reviews.llvm.org/D25326#564283, @NoQ wrote:
>
> > In https://reviews.llvm.org/D25326#564239, @danielmarjamaki wrote:
> >
> > > ok. As far as I see it's not trivial to know which ReturnStmt there was 
> > > when CallExitBegin is created.
> >
> >
> > We're in `HandleBlockEdge`, just pass down the statement from CFG here?
>
>
> I don't directly see how you mean. Code is:
>
>   void CoreEngine::HandleBlockEdge(const BlockEdge , ExplodedNode *Pred) {
>  
> const CFGBlock *Blk = L.getDst();
>
>
> The Blk->dump() says:
>
>   [B0 (EXIT)]
>  Preds (2): B1 B2
>


Sorry... I think I see.   L.getSrc()  will give me the cfg block I am 
interested in.


Repository:
  rL LLVM

https://reviews.llvm.org/D25326



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D25326: [StaticAnalyser] Don't merge different returns in ExplodedGraph

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a comment.

In https://reviews.llvm.org/D25326#564283, @NoQ wrote:

> In https://reviews.llvm.org/D25326#564239, @danielmarjamaki wrote:
>
> > ok. As far as I see it's not trivial to know which ReturnStmt there was 
> > when CallExitBegin is created.
>
>
> We're in `HandleBlockEdge`, just pass down the statement from CFG here?


I don't directly see how you mean. Code is:

  void CoreEngine::HandleBlockEdge(const BlockEdge , ExplodedNode *Pred) {
  
const CFGBlock *Blk = L.getDst();

The Blk->dump() says:

  [B0 (EXIT)]
 Preds (2): B1 B2


Repository:
  rL LLVM

https://reviews.llvm.org/D25326



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D25326: [StaticAnalyser] Don't merge different returns in ExplodedGraph

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a comment.

ok. As far as I see it's not trivial to know which ReturnStmt there was when 
CallExitBegin is created. Do you suggest that I move it or that I try to lookup 
the ReturnStmt? I guess it can be looked up by looking in the predecessors in 
the ExplodedGraph?

> Finally, i'm not quite sure why CallExitBegin is at all necessary. I wonder 
> if we could just remove it and jump straight to Bind Return Value, also need 
> to think.

.. unless that is better apprach


Repository:
  rL LLVM

https://reviews.llvm.org/D25326



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D25326: [StaticAnalyser] Don't merge different returns in ExplodedGraph

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a comment.

In https://reviews.llvm.org/D25326#564082, @NoQ wrote:

> Funny facts about the Environment:
>
> (a) Environment allows taking SVals of ReturnStmt, which is not an 
> expression, by transparently converting it into its sub-expression. In fact, 
> it only stores expressions.
>
> Having just noticed (a), i also understand that (a) is not of direct 
> importance to the question, however:
>
> (b)  With a similar trick, Environment allows taking SVal of literal 
> expressions, but never stores literal expressions. Instead, it reconstructs 
> the constant value by looking at the literal and returns the freshly 
> constructed value when asked.
>
> This leads to "return 0;" and "return 1;" branches having the same program 
> state. The difference should have been within Environment (return values are 
> different), however return values are literals, and they aren't stored in the 
> Environment, and hence the Environment is equal in both states. If only the 
> function returned, say, 0 and i, the problem wouldn't have been there.


I did not know "a" and "b", thanks for info.

> This leaves us with two options:
> 
> 1. Tell the Environment to store everything. This would make things heavy. 
> However, i do not completely understand the consequences of this bug at the 
> moment - perhaps there would be more problems due to this state merging 
> unless we start storing everything.
> 2. Rely on the ProgramPoint to remember where we are. After all, why do we 
> merge when program points should clearly be different? The program point that 
> fails us is CallExitBegin - we could construct it with ReturnStmt and its 
> block count to discriminate between different returns. It should fix this 
> issue, and is similar to the approach taken in this patch (just puts things 
> to their own place), however, as i mentioned, there might be more problems 
> with misbehavior (b) of the Environment, need to think.



1. yes sounds heavy.
2. I assume you are right.. however as I understand it the ProgramPoint when 
CallExitBegin is called is the same (in the exit block). do you suggest that we 
take the ProgramPoint for the exit block's predecessor?

> Finally, i'm not quite sure why CallExitBegin is at all necessary. I wonder 
> if we could just remove it and jump straight to Bind Return Value, also need 
> to think.

me too. however there is much I wonder about when it comes to ExplodedGraph. :-)


Repository:
  rL LLVM

https://reviews.llvm.org/D25326



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D25326: [StaticAnalyser] Don't merge different returns in ExplodedGraph

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added reviewers: NoQ, zaks.anna, dcoughlin, a.sidorin, 
xazax.hun.
danielmarjamaki added a comment.

adding reviewers.


Repository:
  rL LLVM

https://reviews.llvm.org/D25326



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D25326: [StaticAnalyser] Don't merge different returns in ExplodedGraph

[Daniel Marjamäki via cfe-commits]

danielmarjamaki created this revision.
danielmarjamaki added subscribers: cfe-commits, dcoughlin, NoQ.
danielmarjamaki set the repository for this revision to rL LLVM.

Returns when calling an inline function should not be merged in the 
ExplodedGraph unless they are same.

Background post on cfe-dev:
http://lists.llvm.org/pipermail/cfe-dev/2016-October/051001.html

Here is an example patch that solves my false positives and also fixes 2 false 
negatives in existing tests.

What do you think about this approach?


Repository:
  rL LLVM

https://reviews.llvm.org/D25326

Files:
  lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  test/Analysis/inlining/InlineObjCClassMethod.m
  test/Analysis/unreachable-code-path.c


Index: test/Analysis/unreachable-code-path.c
===
--- test/Analysis/unreachable-code-path.c
+++ test/Analysis/unreachable-code-path.c
@@ -194,3 +194,14 @@
 break;
   }
 }
+
+extern int table[];
+static int inlineFunction(const int i) {
+  if (table[i] != 0) // <- SVal for table[0] is unknown
+return 1;
+  return 0;
+}
+void test13(int i) {
+  int x = inlineFunction(i);
+  x && x < 10;
+}
Index: test/Analysis/inlining/InlineObjCClassMethod.m
===
--- test/Analysis/inlining/InlineObjCClassMethod.m
+++ test/Analysis/inlining/InlineObjCClassMethod.m
@@ -174,12 +174,12 @@
 @implementation MyClassSelf
 + (int)testClassMethodByKnownVarDecl {
   int y = [MyParentSelf testSelf];
-  return 5/y; // Should warn here.
+  return 5/y; // expected-warning{{Division by zero}}
 }
 @end
 int foo2() {
   int y = [MyParentSelf testSelf];
-  return 5/y; // Should warn here.
+  return 5/y; // expected-warning{{Division by zero}}
 }
 
 // TODO: We do not inline 'getNum' in the following case, where the value of 
Index: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -23,6 +23,8 @@
 #include "llvm/ADT/Statistic.h"
 #include "llvm/Support/SaveAndRestore.h"
 
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastStmt, const void *)
+
 using namespace clang;
 using namespace ento;
 
@@ -986,7 +988,8 @@
   if (RS->getRetValue()) {
 for (ExplodedNodeSet::iterator it = dstPreVisit.begin(),
   ei = dstPreVisit.end(); it != ei; ++it) {
-  B.generateNode(RS, *it, (*it)->getState());
+  ProgramStateRef State = (*it)->getState();
+  B.generateNode(RS, *it, State->set((const void*)RS));
 }
   }
 }


Index: test/Analysis/unreachable-code-path.c
===
--- test/Analysis/unreachable-code-path.c
+++ test/Analysis/unreachable-code-path.c
@@ -194,3 +194,14 @@
 break;
   }
 }
+
+extern int table[];
+static int inlineFunction(const int i) {
+  if (table[i] != 0) // <- SVal for table[0] is unknown
+return 1;
+  return 0;
+}
+void test13(int i) {
+  int x = inlineFunction(i);
+  x && x < 10;
+}
Index: test/Analysis/inlining/InlineObjCClassMethod.m
===
--- test/Analysis/inlining/InlineObjCClassMethod.m
+++ test/Analysis/inlining/InlineObjCClassMethod.m
@@ -174,12 +174,12 @@
 @implementation MyClassSelf
 + (int)testClassMethodByKnownVarDecl {
   int y = [MyParentSelf testSelf];
-  return 5/y; // Should warn here.
+  return 5/y; // expected-warning{{Division by zero}}
 }
 @end
 int foo2() {
   int y = [MyParentSelf testSelf];
-  return 5/y; // Should warn here.
+  return 5/y; // expected-warning{{Division by zero}}
 }
 
 // TODO: We do not inline 'getNum' in the following case, where the value of 
Index: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -23,6 +23,8 @@
 #include "llvm/ADT/Statistic.h"
 #include "llvm/Support/SaveAndRestore.h"
 
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastStmt, const void *)
+
 using namespace clang;
 using namespace ento;
 
@@ -986,7 +988,8 @@
   if (RS->getRetValue()) {
 for (ExplodedNodeSet::iterator it = dstPreVisit.begin(),
   ei = dstPreVisit.end(); it != ei; ++it) {
-  B.generateNode(RS, *it, (*it)->getState());
+  ProgramStateRef State = (*it)->getState();
+  B.generateNode(RS, *it, State->set((const void*)RS));
 }
   }
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D19586: Misleading Indentation check

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added inline comments.


> MisleadingIndentationCheck.cpp:20
> +
> +void MisleadingIndentationCheck::danglingElseCheck(
> +const MatchFinder::MatchResult ) {

There is no handling of tabs and spaces by danglingElseCheck as far as I see.

The "if" might for example be indented with spaces. And then the corresponding 
"else" is indented with a tab. Then I guess there is false positive.

If the "if" is indented with 2 spaces and the "else" is indented with 2 tabs. 
then I assume there is false negative.

It's unfortunate. Is it ok?

> MisleadingIndentationCheck.cpp:34
> +  Result.SourceManager->getExpansionColumnNumber(ElseLoc))
> +diag(ElseLoc, "potential dangling else");
> +}

I see no test case that says "potential dangling else". If I'm not mistaken 
that should be added.

Is it really sufficient to write that? It's not obvious to me why clang-tidy 
would think it's dangling.

> MisleadingIndentationCheck.cpp:44
> +
> +if (isa(CurrentStmt)) {
> +  IfStmt *CurrentIf = dyn_cast(CurrentStmt);

You don't need to use both isa and dyn_cast:

  if (const auto *CurrentIf = dyn_cast(CurrentStmt)) {
  Inside = CurrentIf->getElse() ? CurrentIf->getElse() : 
CurrentIf->getThen();
  } 

> MisleadingIndentationCheck.h:20
> +/// Checks the code for dangling else,
> +///   and possible misleading indentations due to missing braces.
> +///

there are too many spaces in this comment

https://reviews.llvm.org/D19586



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D24861: [Sema] extend Wshift-op-parentheses so it warns for multiplicative operators

[Daniel Marjamäki via cfe-commits]

danielmarjamaki set the repository for this revision to rL LLVM.
danielmarjamaki updated this revision to Diff 73633.
danielmarjamaki added a comment.

Add new flag : -Wshift-op-parentheses-mul
This is not enabled by default.


Repository:
  rL LLVM

https://reviews.llvm.org/D24861

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaExpr.cpp
  test/Sema/parentheses.cpp

Index: test/Sema/parentheses.cpp
===
--- test/Sema/parentheses.cpp
+++ test/Sema/parentheses.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -Wparentheses -fsyntax-only -verify %s
-// RUN: %clang_cc1 -Wparentheses -fsyntax-only -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -Wparentheses -Wshift-op-parentheses-mul -fsyntax-only -verify %s
+// RUN: %clang_cc1 -Wparentheses -Wshift-op-parentheses-mul -fsyntax-only -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
 
 bool someConditionFunc();
 
@@ -95,6 +95,23 @@
   // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:10}:"("
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:15-[[@LINE-3]]:15}:")"
 
+  (void)(a >> b * c); // expected-warning {{operator '>>' has lower precedence than '*'; '*' will be evaluated first}} \
+ expected-note {{place parentheses around the '*' expression to silence this warning}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:15-[[@LINE-2]]:15}:"("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:20-[[@LINE-3]]:20}:")"
+
+  (void)(a / b << c); // expected-warning {{operator '<<' has lower precedence than '/'; '/' will be evaluated first}} \
+ expected-note {{place parentheses around the '/' expression to silence this warning}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:10}:"("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:15-[[@LINE-3]]:15}:")"
+
+  (void)(a % b << c); // expected-warning {{operator '<<' has lower precedence than '%'; '%' will be evaluated first}} \
+ expected-note {{place parentheses around the '%' expression to silence this warning}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:10}:"("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:15-[[@LINE-3]]:15}:")"
+
+  (void)(a * b << c); // no warning, often the execution order does not matter.
+
   Stream() << b + c;
   Stream() >> b + c; // expected-warning {{operator '>>' has lower precedence than '+'; '+' will be evaluated first}} \
 expected-note {{place parentheses around the '+' expression to silence this warning}}
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -11263,18 +11263,26 @@
   }
 }
 
-static void DiagnoseAdditionInShift(Sema , SourceLocation OpLoc,
-Expr *SubExpr, StringRef Shift) {
-  if (BinaryOperator *Bop = dyn_cast(SubExpr)) {
-if (Bop->getOpcode() == BO_Add || Bop->getOpcode() == BO_Sub) {
-  StringRef Op = Bop->getOpcodeStr();
-  S.Diag(Bop->getOperatorLoc(), diag::warn_addition_in_bitshift)
-  << Bop->getSourceRange() << OpLoc << Shift << Op;
-  SuggestParentheses(S, Bop->getOperatorLoc(),
-  S.PDiag(diag::note_precedence_silence) << Op,
-  Bop->getSourceRange());
-}
-  }
+static void DiagnoseAddOrMulInShift(Sema , SourceLocation OpLoc,
+Expr *SubExpr, StringRef Shift,
+bool ShiftLeftLHS) {
+  BinaryOperator *Bop = dyn_cast(SubExpr);
+  if (!Bop)
+return;
+  if (!Bop->isAdditiveOp() && !Bop->isMultiplicativeOp())
+return;
+  // In many cases, execution order does not matter for 'A*B<getOpcode() == BO_Mul)
+return;
+
+  StringRef Op = Bop->getOpcodeStr();
+  S.Diag(Bop->getOperatorLoc(), Bop->isAdditiveOp()
+? diag::warn_addition_in_bitshift
+: diag::warn_multiplication_in_bitshift)
+  << Bop->getSourceRange() << OpLoc << Shift << Op;
+  SuggestParentheses(S, Bop->getOperatorLoc(),
+ S.PDiag(diag::note_precedence_silence) << Op,
+ Bop->getSourceRange());
 }
 
 static void DiagnoseShiftCompare(Sema , SourceLocation OpLoc,
@@ -11330,8 +11338,8 @@
   if ((Opc == BO_Shl && LHSExpr->getType()->isIntegralType(Self.getASTContext()))
   || Opc == BO_Shr) {
 StringRef Shift = BinaryOperator::getOpcodeStr(Opc);
-DiagnoseAdditionInShift(Self, OpLoc, LHSExpr, Shift);
-DiagnoseAdditionInShift(Self, OpLoc, RHSExpr, Shift);
+DiagnoseAddOrMulInShift(Self, OpLoc, LHSExpr, Shift, Opc == BO_Shl);
+DiagnoseAddOrMulInShift(Self, OpLoc, RHSExpr, Shift, false);
   }
 
   // Warn on overloaded shift operators and comparisons, such as:
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- 

[PATCH] D24656: [clang-tidy] Add check readability-redundant-declaration

[Daniel Marjamäki via cfe-commits]

danielmarjamaki marked 2 inline comments as done.
danielmarjamaki added a comment.

Ping.

I am not very happy about how I detect multivariable declarations. But I really 
don't see any such info in the VarDecl. For instance, the AST dump does not 
show this info.


https://reviews.llvm.org/D24656



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

SV: [PATCH] D24861: [Sema] extend Wshift-op-parentheses so it warns for multiplicative operators

[Daniel Marjamäki via cfe-commits]

Hello!

Moving it to a subgroup such as -Wparentheses is fine for me. I will look at 
that when I have time. Do you think this warning has an acceptable output then?

Best regards,
Daniel Marjamäki

..
Daniel Marjamäki Senior Engineer
Evidente ES East AB  Warfvinges väg 34  SE-112 51 Stockholm  Sweden

Mobile: +46 (0)709 12 42 62
E-mail: daniel.marjam...@evidente.se

www.evidente.se


Från: Richard Trieu [rtr...@google.com]
Skickat: den 1 oktober 2016 00:41
Till: reviews+d24861+public+1ab88c6ac93f5...@reviews.llvm.org
Kopia: Daniel Marjamäki; David Blaikie; jo...@netbsd.org; 
bruno.card...@gmail.com; cfe-commits
Ämne: Re: [PATCH] D24861: [Sema] extend Wshift-op-parentheses so it warns for 
multiplicative operators

Currently, this warning is on by default.  As you said, the results you found 
look intentional in many cases, so there is a high false positive rate.  For on 
by default warnings, we expect a high true positive rate and intend for users 
to not disable the warning.  From my analysis on a separate codebase, I found 
less than 10% true positive rate out of 200 warnings.  One option might be to 
move this warning to a subgroup, which would leave it discoverable from either 
-Wall or -Wparentheses, but not have it on by default.

On Wed, Sep 28, 2016 at 4:09 AM, Daniel Marjamäki 
> wrote:
danielmarjamaki added a comment.

I updated the patch so it does not warn about 'A * B << C'. It's a simple fix. 
I have not made careful measurements but I guess that the performance penalty 
is acceptable.


https://reviews.llvm.org/D24861




___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D24905: Fix unreachable code false positive, vardecl in switch

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a comment.

In https://reviews.llvm.org/D24905#557573, @dcoughlin wrote:

> Sorry, missed this patch.
>
> I think it would good to add a test to make sure we do warn when the var decl 
> has an initializer, since that will not be executed.
>
>   void varDecl(int X) {
> switch (X) {
>   int A = 12; // We do want a warning here, since the variable will be 
> uninitialized in C (This is not allowed in C++).
> case 1:
>   ...
>   break;
> }
>   }
>


I added such test case with 283096.


Repository:
  rL LLVM

https://reviews.llvm.org/D24905



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D24759: [RFC][StaticAnalyser] fix unreachable code false positives when block numbers mismatch

[Daniel Marjamäki via cfe-commits]

danielmarjamaki closed this revision.
danielmarjamaki added a comment.

r283095


https://reviews.llvm.org/D24759



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24656: [clang-tidy] Add check readability-redundant-declaration

[Daniel Marjamäki via cfe-commits]

danielmarjamaki marked 3 inline comments as done.


Comment at: clang-tidy/readability/RedundantDeclarationCheck.cpp:39
@@ +38,3 @@
+  bool MultiVar = false;
+  if (const auto *VD = dyn_cast(D)) {
+if (VD && VD->getPreviousDecl()->getStorageClass() == SC_Extern &&

I don't want to generate a fixit. because it could easily break the code. And 
it will probably depend on inclusion order.


Comment at: clang-tidy/readability/RedundantDeclarationCheck.cpp:45
@@ +44,3 @@
+for (const auto Other : VD->getDeclContext()->decls()) {
+  if (Other != D && Other->getLocStart() == VD->getLocStart()) {
+MultiVar = true;

I think this is better. But not perfect. Maybe there is still a better way.


Comment at: clang-tidy/readability/RedundantDeclarationCheck.cpp:59-65
@@ +58,9 @@
+  {
+auto Diag = diag(D->getLocation(), "redundant '%0' declaration")
+<< cast(D)->getName();
+if (!MultiVar && !DifferentHeaders)
+  Diag << FixItHint::CreateRemoval(
+   SourceRange(D->getSourceRange().getBegin(), EndLoc));
+  }
+  diag(Prev->getLocation(), "previously declared here", DiagnosticIDs::Note);
+}

Thanks! That works.


https://reviews.llvm.org/D24656



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24656: [clang-tidy] Add check readability-redundant-declaration

[Daniel Marjamäki via cfe-commits]

danielmarjamaki updated this revision to Diff 72802.
danielmarjamaki added a comment.

Fix review comments


https://reviews.llvm.org/D24656

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tidy/readability/RedundantDeclarationCheck.cpp
  clang-tidy/readability/RedundantDeclarationCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-redundant-declaration.rst
  test/clang-tidy/readability-redundant-declaration.cpp

Index: test/clang-tidy/readability-redundant-declaration.cpp
===
--- test/clang-tidy/readability-redundant-declaration.cpp
+++ test/clang-tidy/readability-redundant-declaration.cpp
@@ -0,0 +1,23 @@
+// RUN: %check_clang_tidy %s readability-redundant-declaration %t
+
+extern int Xyz;
+extern int Xyz;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: redundant 'Xyz' declaration [readability-redundant-declaration]
+// CHECK-FIXES: {{^}}{{$}}
+int Xyz = 123;
+
+extern int A;
+extern int A, B;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: redundant 'A' declaration
+// CHECK-FIXES: {{^}}extern int A, B;{{$}}
+
+extern int Buf[10];
+extern int Buf[10];
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: redundant 'Buf' declaration
+// CHECK-FIXES: {{^}}{{$}}
+
+static int f();
+static int f();
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: redundant 'f' declaration
+// CHECK-FIXES: {{^}}{{$}}
+static int f() {}
Index: docs/clang-tidy/checks/readability-redundant-declaration.rst
===
--- docs/clang-tidy/checks/readability-redundant-declaration.rst
+++ docs/clang-tidy/checks/readability-redundant-declaration.rst
@@ -0,0 +1,17 @@
+.. title:: clang-tidy - readability-redundant-declaration
+
+readability-redundant-declaration
+=
+
+Finds redundant variable declarations.
+
+.. code-block:: c++
+
+  extern int X;
+  extern int X;
+
+becomes
+
+.. code-block:: c++
+
+  extern int X;
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -133,6 +133,7 @@
readability-named-parameter
readability-non-const-parameter
readability-redundant-control-flow
+   readability-redundant-declaration
readability-redundant-smartptr-get
readability-redundant-string-cstr
readability-redundant-string-init
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -111,6 +111,11 @@
   Flags function parameters of a pointer type that could be changed to point to
   a constant type instead.
 
+- New `readability-redundant-declaration
+  `_ check
+
+  Warns about duplicate variable declarations.
+
 Fixed bugs:
 
 - `modernize-make-unique
Index: clang-tidy/readability/RedundantDeclarationCheck.h
===
--- clang-tidy/readability/RedundantDeclarationCheck.h
+++ clang-tidy/readability/RedundantDeclarationCheck.h
@@ -0,0 +1,35 @@
+//===--- RedundantDeclarationCheck.h - clang-tidy*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANT_DECLARATION_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANT_DECLARATION_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Find redundant variable declarations.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-redundant-declaration.html
+class RedundantDeclarationCheck : public ClangTidyCheck {
+public:
+  RedundantDeclarationCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult ) override;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANT_DECLARATION_H
Index: clang-tidy/readability/RedundantDeclarationCheck.cpp
===
--- clang-tidy/readability/RedundantDeclarationCheck.cpp
+++ clang-tidy/readability/RedundantDeclarationCheck.cpp
@@ -0,0 +1,70 @@
+//===--- RedundantDeclarationCheck.cpp - clang-tidy===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed 

Re: [PATCH] D24861: [Sema] extend Wshift-op-parentheses so it warns for multiplicative operators

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a comment.

I updated the patch so it does not warn about 'A * B << C'. It's a simple fix. 
I have not made careful measurements but I guess that the performance penalty 
is acceptable.


https://reviews.llvm.org/D24861



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24861: [Sema] extend Wshift-op-parentheses so it warns for multiplicative operators

[Daniel Marjamäki via cfe-commits]

danielmarjamaki removed rL LLVM as the repository for this revision.
danielmarjamaki updated this revision to Diff 72797.
danielmarjamaki added a comment.

Don't write warning for multiplication in LHS of <<. Often the execution order 
is not important.


https://reviews.llvm.org/D24861

Files:
  lib/Sema/SemaExpr.cpp
  test/Sema/parentheses.cpp

Index: test/Sema/parentheses.cpp
===
--- test/Sema/parentheses.cpp
+++ test/Sema/parentheses.cpp
@@ -95,6 +95,23 @@
   // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:10}:"("
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:15-[[@LINE-3]]:15}:")"
 
+  (void)(a >> b * c); // expected-warning {{operator '>>' has lower precedence 
than '*'; '*' will be evaluated first}} \
+ expected-note {{place parentheses around the '*' 
expression to silence this warning}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:15-[[@LINE-2]]:15}:"("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:20-[[@LINE-3]]:20}:")"
+
+  (void)(a / b << c); // expected-warning {{operator '<<' has lower precedence 
than '/'; '/' will be evaluated first}} \
+ expected-note {{place parentheses around the '/' 
expression to silence this warning}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:10}:"("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:15-[[@LINE-3]]:15}:")"
+
+  (void)(a % b << c); // expected-warning {{operator '<<' has lower precedence 
than '%'; '%' will be evaluated first}} \
+ expected-note {{place parentheses around the '%' 
expression to silence this warning}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:10}:"("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:15-[[@LINE-3]]:15}:")"
+
+  (void)(a * b << c); // no warning, often the execution order does not matter.
+
   Stream() << b + c;
   Stream() >> b + c; // expected-warning {{operator '>>' has lower precedence 
than '+'; '+' will be evaluated first}} \
 expected-note {{place parentheses around the '+' 
expression to silence this warning}}
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -11246,18 +11246,24 @@
   }
 }
 
-static void DiagnoseAdditionInShift(Sema , SourceLocation OpLoc,
-Expr *SubExpr, StringRef Shift) {
-  if (BinaryOperator *Bop = dyn_cast(SubExpr)) {
-if (Bop->getOpcode() == BO_Add || Bop->getOpcode() == BO_Sub) {
-  StringRef Op = Bop->getOpcodeStr();
-  S.Diag(Bop->getOperatorLoc(), diag::warn_addition_in_bitshift)
-  << Bop->getSourceRange() << OpLoc << Shift << Op;
-  SuggestParentheses(S, Bop->getOperatorLoc(),
-  S.PDiag(diag::note_precedence_silence) << Op,
-  Bop->getSourceRange());
-}
-  }
+static void DiagnoseAddOrMulInShift(Sema , SourceLocation OpLoc,
+Expr *SubExpr, StringRef Shift,
+bool ShiftLeftLHS) {
+  BinaryOperator *Bop = dyn_cast(SubExpr);
+  if (!Bop)
+return;
+  if (!Bop->isAdditiveOp() && !Bop->isMultiplicativeOp())
+return;
+  // In many cases, execution order does not matter for 'A*B<getOpcode() == BO_Mul)
+return;
+
+  StringRef Op = Bop->getOpcodeStr();
+  S.Diag(Bop->getOperatorLoc(), diag::warn_addition_in_bitshift)
+  << Bop->getSourceRange() << OpLoc << Shift << Op;
+  SuggestParentheses(S, Bop->getOperatorLoc(),
+ S.PDiag(diag::note_precedence_silence) << Op,
+ Bop->getSourceRange());
 }
 
 static void DiagnoseShiftCompare(Sema , SourceLocation OpLoc,
@@ -11313,8 +11319,8 @@
   if ((Opc == BO_Shl && 
LHSExpr->getType()->isIntegralType(Self.getASTContext()))
   || Opc == BO_Shr) {
 StringRef Shift = BinaryOperator::getOpcodeStr(Opc);
-DiagnoseAdditionInShift(Self, OpLoc, LHSExpr, Shift);
-DiagnoseAdditionInShift(Self, OpLoc, RHSExpr, Shift);
+DiagnoseAddOrMulInShift(Self, OpLoc, LHSExpr, Shift, Opc == BO_Shl);
+DiagnoseAddOrMulInShift(Self, OpLoc, RHSExpr, Shift, false);
   }
 
   // Warn on overloaded shift operators and comparisons, such as:


Index: test/Sema/parentheses.cpp
===
--- test/Sema/parentheses.cpp
+++ test/Sema/parentheses.cpp
@@ -95,6 +95,23 @@
   // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:10}:"("
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:15-[[@LINE-3]]:15}:")"
 
+  (void)(a >> b * c); // expected-warning {{operator '>>' has lower precedence than '*'; '*' will be evaluated first}} \
+ expected-note {{place parentheses around the '*' expression to silence this warning}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:15-[[@LINE-2]]:15}:"("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:20-[[@LINE-3]]:20}:")"
+
+  (void)(a / b << c); // expected-warning {{operator '<<' 

Re: [PATCH] D24905: Fix unreachable code false positive, vardecl in switch

[Daniel Marjamäki via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL282574: [StaticAnalyzer] Fix false positives for vardecls 
that are technically… (authored by danielmarjamaki).

Changed prior to commit:
  https://reviews.llvm.org/D24905?vs=72775=72793#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D24905

Files:
  cfe/trunk/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
  cfe/trunk/test/Analysis/unreachable-code-path.c

Index: cfe/trunk/test/Analysis/unreachable-code-path.c
===
--- cfe/trunk/test/Analysis/unreachable-code-path.c
+++ cfe/trunk/test/Analysis/unreachable-code-path.c
@@ -158,3 +158,18 @@
 }
   }
 }
+
+// Don't warn about unreachable VarDecl.
+void dostuff(int*A);
+void varDecl(int X) {
+  switch (X) {
+int A; // No warning here.
+  case 1:
+dostuff();
+break;
+  case 2:
+dostuff();
+break;
+  }
+}
+
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
@@ -191,8 +191,10 @@
 // Find the Stmt* in a CFGBlock for reporting a warning
 const Stmt *UnreachableCodeChecker::getUnreachableStmt(const CFGBlock *CB) {
   for (CFGBlock::const_iterator I = CB->begin(), E = CB->end(); I != E; ++I) {
-if (Optional S = I->getAs())
-  return S->getStmt();
+if (Optional S = I->getAs()) {
+  if (!isa(S->getStmt()))
+return S->getStmt();
+}
   }
   if (const Stmt *S = CB->getTerminator())
 return S;


Index: cfe/trunk/test/Analysis/unreachable-code-path.c
===
--- cfe/trunk/test/Analysis/unreachable-code-path.c
+++ cfe/trunk/test/Analysis/unreachable-code-path.c
@@ -158,3 +158,18 @@
 }
   }
 }
+
+// Don't warn about unreachable VarDecl.
+void dostuff(int*A);
+void varDecl(int X) {
+  switch (X) {
+int A; // No warning here.
+  case 1:
+dostuff();
+break;
+  case 2:
+dostuff();
+break;
+  }
+}
+
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
@@ -191,8 +191,10 @@
 // Find the Stmt* in a CFGBlock for reporting a warning
 const Stmt *UnreachableCodeChecker::getUnreachableStmt(const CFGBlock *CB) {
   for (CFGBlock::const_iterator I = CB->begin(), E = CB->end(); I != E; ++I) {
-if (Optional S = I->getAs())
-  return S->getStmt();
+if (Optional S = I->getAs()) {
+  if (!isa(S->getStmt()))
+return S->getStmt();
+}
   }
   if (const Stmt *S = CB->getTerminator())
 return S;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24905: Fix unreachable code false positive, vardecl in switch

[Daniel Marjamäki via cfe-commits]

danielmarjamaki marked an inline comment as done.
danielmarjamaki added a comment.

https://reviews.llvm.org/D24905



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24905: Fix unreachable code false positive, vardecl in switch

[Daniel Marjamäki via cfe-commits]

danielmarjamaki marked an inline comment as done.


Comment at: lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp:195
@@ +194,3 @@
+if (Optional S = I->getAs()) {
+  if (!isa(S->getStmt()))
+return S->getStmt();

yes I agree.


https://reviews.llvm.org/D24905



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24905: Fix unreachable code false positive, vardecl in switch

[Daniel Marjamäki via cfe-commits]

danielmarjamaki removed rL LLVM as the repository for this revision.
danielmarjamaki updated this revision to Diff 72775.
danielmarjamaki added a comment.

Use !isa. Suggestion by Gabor.


https://reviews.llvm.org/D24905

Files:
  lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
  test/Analysis/unreachable-code-path.c

Index: test/Analysis/unreachable-code-path.c
===
--- test/Analysis/unreachable-code-path.c
+++ test/Analysis/unreachable-code-path.c
@@ -158,3 +158,18 @@
 }
   }
 }
+
+// Don't warn about unreachable VarDecl.
+void dostuff(int*A);
+void varDecl(int X) {
+  switch (X) {
+int A; // No warning here.
+  case 1:
+dostuff();
+break;
+  case 2:
+dostuff();
+break;
+  }
+}
+
Index: lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
@@ -191,8 +191,10 @@
 // Find the Stmt* in a CFGBlock for reporting a warning
 const Stmt *UnreachableCodeChecker::getUnreachableStmt(const CFGBlock *CB) {
   for (CFGBlock::const_iterator I = CB->begin(), E = CB->end(); I != E; ++I) {
-if (Optional S = I->getAs())
-  return S->getStmt();
+if (Optional S = I->getAs()) {
+  if (!isa(S->getStmt()))
+return S->getStmt();
+}
   }
   if (const Stmt *S = CB->getTerminator())
 return S;


Index: test/Analysis/unreachable-code-path.c
===
--- test/Analysis/unreachable-code-path.c
+++ test/Analysis/unreachable-code-path.c
@@ -158,3 +158,18 @@
 }
   }
 }
+
+// Don't warn about unreachable VarDecl.
+void dostuff(int*A);
+void varDecl(int X) {
+  switch (X) {
+int A; // No warning here.
+  case 1:
+dostuff();
+break;
+  case 2:
+dostuff();
+break;
+  }
+}
+
Index: lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
@@ -191,8 +191,10 @@
 // Find the Stmt* in a CFGBlock for reporting a warning
 const Stmt *UnreachableCodeChecker::getUnreachableStmt(const CFGBlock *CB) {
   for (CFGBlock::const_iterator I = CB->begin(), E = CB->end(); I != E; ++I) {
-if (Optional S = I->getAs())
-  return S->getStmt();
+if (Optional S = I->getAs()) {
+  if (!isa(S->getStmt()))
+return S->getStmt();
+}
   }
   if (const Stmt *S = CB->getTerminator())
 return S;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24861: [Sema] extend Wshift-op-parentheses so it warns for multiplicative operators

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a comment.

Compiling 2064 projects resulted in 904 warnings

Here are the results:
https://drive.google.com/file/d/0BykPmWrCOxt2N04tYl8zVHA3MXc/view?usp=sharing

The results looks acceptable imho. The code looks intentional in many cases so 
I believe there are users that will disable this warning. Probably there are 
true positives where the evaluation order is not really known. There were many 
warnings about macro arguments where the macro bitshifts the argument - these 
macros look very shaky to me.

I saw some warnings about such code:

  a * b << c

Maybe we should not warn about this. As far as I see, the result will be the 
same if (a*b) or (b<

Re: [PATCH] D24759: [RFC][StaticAnalyser] fix unreachable code false positives when block numbers mismatch

[Daniel Marjamäki via cfe-commits]

danielmarjamaki updated this revision to Diff 72461.
danielmarjamaki marked 2 inline comments as done.
danielmarjamaki added a comment.

Fixed review comments by Devin. It works better now!


https://reviews.llvm.org/D24759

Files:
  lib/Analysis/CFG.cpp
  test/Analysis/unreachable-code-path.c

Index: test/Analysis/unreachable-code-path.c
===
--- test/Analysis/unreachable-code-path.c
+++ test/Analysis/unreachable-code-path.c
@@ -158,3 +158,14 @@
 }
   }
 }
+
+// Ensure that ExplodedGraph and unoptimized CFG match.
+void test12(int x) {
+  switch (x) {
+  case 1:
+break; // not unreachable
+  case 2:
+do { } while (0);
+break;
+  }
+}
Index: lib/Analysis/CFG.cpp
===
--- lib/Analysis/CFG.cpp
+++ lib/Analysis/CFG.cpp
@@ -2983,20 +2983,19 @@
 return nullptr;
 }
 
-if (!KnownVal.isFalse()) {
-  // Add an intermediate block between the BodyBlock and the
-  // ExitConditionBlock to represent the "loop back" transition.  Create an
-  // empty block to represent the transition block for looping back to the
-  // head of the loop.
-  // FIXME: Can we do this more efficiently without adding another block?
-  Block = nullptr;
-  Succ = BodyBlock;
-  CFGBlock *LoopBackBlock = createBlock();
-  LoopBackBlock->setLoopTarget(D);
+// Add an intermediate block between the BodyBlock and the
+// ExitConditionBlock to represent the "loop back" transition.  Create an
+// empty block to represent the transition block for looping back to the
+// head of the loop.
+// FIXME: Can we do this more efficiently without adding another block?
+Block = nullptr;
+Succ = BodyBlock;
+CFGBlock *LoopBackBlock = createBlock();
+LoopBackBlock->setLoopTarget(D);
 
+if (!KnownVal.isFalse())
   // Add the loop body entry as a successor to the condition.
   addSuccessor(ExitConditionBlock, LoopBackBlock);
-}
 else
   addSuccessor(ExitConditionBlock, nullptr);
   }


Index: test/Analysis/unreachable-code-path.c
===
--- test/Analysis/unreachable-code-path.c
+++ test/Analysis/unreachable-code-path.c
@@ -158,3 +158,14 @@
 }
   }
 }
+
+// Ensure that ExplodedGraph and unoptimized CFG match.
+void test12(int x) {
+  switch (x) {
+  case 1:
+break; // not unreachable
+  case 2:
+do { } while (0);
+break;
+  }
+}
Index: lib/Analysis/CFG.cpp
===
--- lib/Analysis/CFG.cpp
+++ lib/Analysis/CFG.cpp
@@ -2983,20 +2983,19 @@
 return nullptr;
 }
 
-if (!KnownVal.isFalse()) {
-  // Add an intermediate block between the BodyBlock and the
-  // ExitConditionBlock to represent the "loop back" transition.  Create an
-  // empty block to represent the transition block for looping back to the
-  // head of the loop.
-  // FIXME: Can we do this more efficiently without adding another block?
-  Block = nullptr;
-  Succ = BodyBlock;
-  CFGBlock *LoopBackBlock = createBlock();
-  LoopBackBlock->setLoopTarget(D);
+// Add an intermediate block between the BodyBlock and the
+// ExitConditionBlock to represent the "loop back" transition.  Create an
+// empty block to represent the transition block for looping back to the
+// head of the loop.
+// FIXME: Can we do this more efficiently without adding another block?
+Block = nullptr;
+Succ = BodyBlock;
+CFGBlock *LoopBackBlock = createBlock();
+LoopBackBlock->setLoopTarget(D);
 
+if (!KnownVal.isFalse())
   // Add the loop body entry as a successor to the condition.
   addSuccessor(ExitConditionBlock, LoopBackBlock);
-}
 else
   addSuccessor(ExitConditionBlock, nullptr);
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24759: [RFC][StaticAnalyser] fix unreachable code false positives when block numbers mismatch

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added inline comments.


Comment at: lib/Analysis/CFG.cpp:2986
@@ -2985,3 +2985,1 @@
 
-if (!KnownVal.isFalse()) {
-  // Add an intermediate block between the BodyBlock and the

dcoughlin wrote:
> You need to keep this check so that the optimized CFG still removes edges 
> that are trivially known to be false.
Thanks


Comment at: lib/Analysis/CFG.cpp:2994
@@ -2993,3 @@
-  Succ = BodyBlock;
-  CFGBlock *LoopBackBlock = createBlock();
-  LoopBackBlock->setLoopTarget(D);

dcoughlin wrote:
> To keep the unoptimized and optimized blocks in sync, this block creation 
> needs to be done regardless of whether the branch condition is known to be 
> false. My advice would be to hoist the creation (and the FIXME comments)  to 
> above the check for whether KnownVal is false.
Thanks


https://reviews.llvm.org/D24759



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D24905: Fix unreachable code false positive, vardecl in switch

[Daniel Marjamäki via cfe-commits]

danielmarjamaki created this revision.
danielmarjamaki added subscribers: cfe-commits, NoQ, zaks.anna, a.sidorin, 
xazax.hun.
danielmarjamaki set the repository for this revision to rL LLVM.

This patch fixes false positives for vardecls that are technically unreachable 
but they are needed.
```
  switch (x) {
int a;  // <- This is unreachable but needed
  case 1:
a = ...
```


For this code there will be Wunused-variable:
```
  if (1+2==45) {
int x;
  }
```

For this code there is 'unreachable code' warning on the 'if (1)':
```
  if (1+2==45) {
int x;
if (1) {}
  }  
```


Repository:
  rL LLVM

https://reviews.llvm.org/D24905

Files:
  lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
  test/Analysis/unreachable-code-path.c

Index: test/Analysis/unreachable-code-path.c
===
--- test/Analysis/unreachable-code-path.c
+++ test/Analysis/unreachable-code-path.c
@@ -158,3 +158,16 @@
 }
   }
 }
+
+// don't warn about unreachable vardecl
+void varDecl() {
+  switch (x) {
+int a; // No warning here.
+  case 1:
+a=1;
+break;
+  case 2:
+a=2;
+break;
+  }
+}
Index: lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
@@ -191,8 +191,10 @@
 // Find the Stmt* in a CFGBlock for reporting a warning
 const Stmt *UnreachableCodeChecker::getUnreachableStmt(const CFGBlock *CB) {
   for (CFGBlock::const_iterator I = CB->begin(), E = CB->end(); I != E; ++I) {
-if (Optional S = I->getAs())
-  return S->getStmt();
+if (Optional S = I->getAs()) {
+  if (isa(S->getStmt()))
+return S->getStmt();
+}
   }
   if (const Stmt *S = CB->getTerminator())
 return S;


Index: test/Analysis/unreachable-code-path.c
===
--- test/Analysis/unreachable-code-path.c
+++ test/Analysis/unreachable-code-path.c
@@ -158,3 +158,16 @@
 }
   }
 }
+
+// don't warn about unreachable vardecl
+void varDecl() {
+  switch (x) {
+int a; // No warning here.
+  case 1:
+a=1;
+break;
+  case 2:
+a=2;
+break;
+  }
+}
Index: lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
@@ -191,8 +191,10 @@
 // Find the Stmt* in a CFGBlock for reporting a warning
 const Stmt *UnreachableCodeChecker::getUnreachableStmt(const CFGBlock *CB) {
   for (CFGBlock::const_iterator I = CB->begin(), E = CB->end(); I != E; ++I) {
-if (Optional S = I->getAs())
-  return S->getStmt();
+if (Optional S = I->getAs()) {
+  if (isa(S->getStmt()))
+return S->getStmt();
+}
   }
   if (const Stmt *S = CB->getTerminator())
 return S;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D24861: [Sema] extend Wshift-op-parentheses so it warns for multiplicative operators

[Daniel Marjamäki via cfe-commits]

danielmarjamaki created this revision.
danielmarjamaki added reviewers: dblaikie, rtrieu.
danielmarjamaki added a subscriber: cfe-commits.
danielmarjamaki set the repository for this revision to rL LLVM.

This patch makes Clang warn about following code:

a = (b * c >> 2);

It might be a good idea to use parentheses to clarify the operator precedence.


Repository:
  rL LLVM

https://reviews.llvm.org/D24861

Files:
  lib/Sema/SemaExpr.cpp
  test/Sema/parentheses.cpp

Index: test/Sema/parentheses.cpp
===
--- test/Sema/parentheses.cpp
+++ test/Sema/parentheses.cpp
@@ -95,6 +95,21 @@
   // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:10}:"("
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:15-[[@LINE-3]]:15}:")"
 
+  (void)(a >> b * c); // expected-warning {{operator '>>' has lower precedence 
than '*'; '*' will be evaluated first}} \
+ expected-note {{place parentheses around the '*' 
expression to silence this warning}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:15-[[@LINE-2]]:15}:"("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:20-[[@LINE-3]]:20}:")"
+
+  (void)(a / b << c); // expected-warning {{operator '<<' has lower precedence 
than '/'; '/' will be evaluated first}} \
+ expected-note {{place parentheses around the '/' 
expression to silence this warning}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:10}:"("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:15-[[@LINE-3]]:15}:")"
+
+  (void)(a % b << c); // expected-warning {{operator '<<' has lower precedence 
than '%'; '%' will be evaluated first}} \
+ expected-note {{place parentheses around the '%' 
expression to silence this warning}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:10}:"("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:15-[[@LINE-3]]:15}:")"
+
   Stream() << b + c;
   Stream() >> b + c; // expected-warning {{operator '>>' has lower precedence 
than '+'; '+' will be evaluated first}} \
 expected-note {{place parentheses around the '+' 
expression to silence this warning}}
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -11246,10 +11246,10 @@
   }
 }
 
-static void DiagnoseAdditionInShift(Sema , SourceLocation OpLoc,
+static void DiagnoseAddOrMulInShift(Sema , SourceLocation OpLoc,
 Expr *SubExpr, StringRef Shift) {
   if (BinaryOperator *Bop = dyn_cast(SubExpr)) {
-if (Bop->getOpcode() == BO_Add || Bop->getOpcode() == BO_Sub) {
+if (Bop->isAdditiveOp() || Bop->isMultiplicativeOp()) {
   StringRef Op = Bop->getOpcodeStr();
   S.Diag(Bop->getOperatorLoc(), diag::warn_addition_in_bitshift)
   << Bop->getSourceRange() << OpLoc << Shift << Op;
@@ -11313,8 +11313,8 @@
   if ((Opc == BO_Shl && 
LHSExpr->getType()->isIntegralType(Self.getASTContext()))
   || Opc == BO_Shr) {
 StringRef Shift = BinaryOperator::getOpcodeStr(Opc);
-DiagnoseAdditionInShift(Self, OpLoc, LHSExpr, Shift);
-DiagnoseAdditionInShift(Self, OpLoc, RHSExpr, Shift);
+DiagnoseAddOrMulInShift(Self, OpLoc, LHSExpr, Shift);
+DiagnoseAddOrMulInShift(Self, OpLoc, RHSExpr, Shift);
   }
 
   // Warn on overloaded shift operators and comparisons, such as:


Index: test/Sema/parentheses.cpp
===
--- test/Sema/parentheses.cpp
+++ test/Sema/parentheses.cpp
@@ -95,6 +95,21 @@
   // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:10}:"("
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:15-[[@LINE-3]]:15}:")"
 
+  (void)(a >> b * c); // expected-warning {{operator '>>' has lower precedence than '*'; '*' will be evaluated first}} \
+ expected-note {{place parentheses around the '*' expression to silence this warning}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:15-[[@LINE-2]]:15}:"("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:20-[[@LINE-3]]:20}:")"
+
+  (void)(a / b << c); // expected-warning {{operator '<<' has lower precedence than '/'; '/' will be evaluated first}} \
+ expected-note {{place parentheses around the '/' expression to silence this warning}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:10}:"("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:15-[[@LINE-3]]:15}:")"
+
+  (void)(a % b << c); // expected-warning {{operator '<<' has lower precedence than '%'; '%' will be evaluated first}} \
+ expected-note {{place parentheses around the '%' expression to silence this warning}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:10}:"("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:15-[[@LINE-3]]:15}:")"
+
   Stream() << b + c;
   Stream() >> b + c; // expected-warning {{operator '>>' has lower precedence than '+'; '+' will be evaluated first}} \
 expected-note {{place 

Re: [PATCH] D16309: Use getCharWidth() instead of magic number

[Daniel Marjamäki via cfe-commits]

danielmarjamaki abandoned this revision.
danielmarjamaki added a comment.

Fixed by r282242


https://reviews.llvm.org/D16309



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24759: [RFC][StaticAnalyser] fix unreachable code false positives when block numbers mismatch

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a comment.

My change is causing a false negative in the 
test/Analysis/uninit-sometimes.cpp. As far as I see my change anyway makes the 
unoptimized CFG better.


https://reviews.llvm.org/D24759



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24759: [RFC][StaticAnalyser] fix unreachable code false positives when block numbers mismatch

[Daniel Marjamäki via cfe-commits]

danielmarjamaki updated this revision to Diff 72252.
danielmarjamaki added a comment.

Updated CFGBuilder::VisitDoStmt


https://reviews.llvm.org/D24759

Files:
  lib/Analysis/CFG.cpp
  test/Analysis/uninit-sometimes.cpp
  test/Analysis/unreachable-code-path.c

Index: test/Analysis/unreachable-code-path.c
===
--- test/Analysis/unreachable-code-path.c
+++ test/Analysis/unreachable-code-path.c
@@ -158,3 +158,14 @@
 }
   }
 }
+
+// Ensure that ExplodedGraph and unoptimized CFG match.
+void test12(int x) {
+  switch (x) {
+  case 1:
+break; // not unreachable
+  case 2:
+do { } while (0);
+break;
+  }
+}
Index: test/Analysis/uninit-sometimes.cpp
===
--- test/Analysis/uninit-sometimes.cpp
+++ test/Analysis/uninit-sometimes.cpp
@@ -374,9 +374,10 @@
 int PR13360(bool b) {
   int x; // expected-note {{variable}}
   if (b) { // expected-warning {{variable 'x' is used uninitialized whenever 
'if' condition is true}} expected-note {{remove}}
-do {
+// TODO: Uncomment "do { } while(0);" below. Warning should still be shown.
+//do {
   foo();
-} while (0);
+//} while (0);
   } else {
 x = 1;
   }
Index: lib/Analysis/CFG.cpp
===
--- lib/Analysis/CFG.cpp
+++ lib/Analysis/CFG.cpp
@@ -2983,22 +2983,18 @@
 return nullptr;
 }
 
-if (!KnownVal.isFalse()) {
-  // Add an intermediate block between the BodyBlock and the
-  // ExitConditionBlock to represent the "loop back" transition.  Create an
-  // empty block to represent the transition block for looping back to the
-  // head of the loop.
-  // FIXME: Can we do this more efficiently without adding another block?
-  Block = nullptr;
-  Succ = BodyBlock;
-  CFGBlock *LoopBackBlock = createBlock();
-  LoopBackBlock->setLoopTarget(D);
+// Add an intermediate block between the BodyBlock and the
+// ExitConditionBlock to represent the "loop back" transition.  Create an
+// empty block to represent the transition block for looping back to the
+// head of the loop.
+// FIXME: Can we do this more efficiently without adding another block?
+Block = nullptr;
+Succ = BodyBlock;
+CFGBlock *LoopBackBlock = createBlock();
+LoopBackBlock->setLoopTarget(D);
 
-  // Add the loop body entry as a successor to the condition.
-  addSuccessor(ExitConditionBlock, LoopBackBlock);
-}
-else
-  addSuccessor(ExitConditionBlock, nullptr);
+// Add the loop body entry as a successor to the condition.
+addSuccessor(ExitConditionBlock, LoopBackBlock);
   }
 
   // Link up the condition block with the code that follows the loop.


Index: test/Analysis/unreachable-code-path.c
===
--- test/Analysis/unreachable-code-path.c
+++ test/Analysis/unreachable-code-path.c
@@ -158,3 +158,14 @@
 }
   }
 }
+
+// Ensure that ExplodedGraph and unoptimized CFG match.
+void test12(int x) {
+  switch (x) {
+  case 1:
+break; // not unreachable
+  case 2:
+do { } while (0);
+break;
+  }
+}
Index: test/Analysis/uninit-sometimes.cpp
===
--- test/Analysis/uninit-sometimes.cpp
+++ test/Analysis/uninit-sometimes.cpp
@@ -374,9 +374,10 @@
 int PR13360(bool b) {
   int x; // expected-note {{variable}}
   if (b) { // expected-warning {{variable 'x' is used uninitialized whenever 'if' condition is true}} expected-note {{remove}}
-do {
+// TODO: Uncomment "do { } while(0);" below. Warning should still be shown.
+//do {
   foo();
-} while (0);
+//} while (0);
   } else {
 x = 1;
   }
Index: lib/Analysis/CFG.cpp
===
--- lib/Analysis/CFG.cpp
+++ lib/Analysis/CFG.cpp
@@ -2983,22 +2983,18 @@
 return nullptr;
 }
 
-if (!KnownVal.isFalse()) {
-  // Add an intermediate block between the BodyBlock and the
-  // ExitConditionBlock to represent the "loop back" transition.  Create an
-  // empty block to represent the transition block for looping back to the
-  // head of the loop.
-  // FIXME: Can we do this more efficiently without adding another block?
-  Block = nullptr;
-  Succ = BodyBlock;
-  CFGBlock *LoopBackBlock = createBlock();
-  LoopBackBlock->setLoopTarget(D);
+// Add an intermediate block between the BodyBlock and the
+// ExitConditionBlock to represent the "loop back" transition.  Create an
+// empty block to represent the transition block for looping back to the
+// head of the loop.
+// FIXME: Can we do this more efficiently without adding another block?
+Block = nullptr;
+Succ = BodyBlock;
+CFGBlock *LoopBackBlock = createBlock();
+LoopBackBlock->setLoopTarget(D);
 
-  

Re: [PATCH] D24507: Add attribute for return values that shouldn't be cast to bool

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a subscriber: danielmarjamaki.


Comment at: include/clang/Basic/AttrDocs.td:2055
@@ -2054,1 +2054,3 @@
 }
+def WarnImpcastToBoolDocs : Documentation {
+  let Category = DocCatFunction;

I saw your email on cfe-dev. This sounds like a good idea to me.


Comment at: lib/StaticAnalyzer/Checkers/ReturnNonBoolChecker.cpp:49
@@ +48,3 @@
+  // needed.
+  if (!State->contains(SR)) return;
+

It seems you need to run clang-format on this file also.


Comment at: test/ReturnNonBoolTest.c:7
@@ +6,3 @@
+
+#ifdef __clang__
+#define RETURNS_NON_BOOL __attribute__((warn_impcast_to_bool))

sorry but why do you have a #ifdef __clang__ isn't it always defined?


Repository:
  rL LLVM

https://reviews.llvm.org/D24507



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16309: Use getCharWidth() instead of magic number

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a comment.

ping.


https://reviews.llvm.org/D16309



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24656: [clang-tidy] Add check readability-redundant-declaration

[Daniel Marjamäki via cfe-commits]

danielmarjamaki marked an inline comment as done.
danielmarjamaki added a comment.

https://reviews.llvm.org/D24656



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24656: [clang-tidy] Add check readability-redundant-declaration

[Daniel Marjamäki via cfe-commits]

danielmarjamaki removed rL LLVM as the repository for this revision.
danielmarjamaki updated this revision to Diff 71775.
danielmarjamaki added a comment.

run clang-format on test. add release notes.


https://reviews.llvm.org/D24656

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tidy/readability/RedundantDeclarationCheck.cpp
  clang-tidy/readability/RedundantDeclarationCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-redundant-declaration.rst
  test/clang-tidy/readability-redundant-declaration.cpp

Index: test/clang-tidy/readability-redundant-declaration.cpp
===
--- test/clang-tidy/readability-redundant-declaration.cpp
+++ test/clang-tidy/readability-redundant-declaration.cpp
@@ -0,0 +1,16 @@
+// RUN: %check_clang_tidy %s readability-redundant-declaration %t
+
+extern int Xyz;
+extern int Xyz;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: redundant variable Xyz declaration [readability-redundant-declaration]
+// CHECK-FIXES: {{^}}{{$}}
+
+extern int A;
+extern int A, B;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: redundant variable A declaration
+// CHECK-FIXES: {{^}}extern int A,B;{{$}}
+
+extern int Buf[10];
+extern int Buf[10];
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: redundant variable Buf declaration
+// CHECK-FIXES: {{^}}{{$}}
Index: docs/clang-tidy/checks/readability-redundant-declaration.rst
===
--- docs/clang-tidy/checks/readability-redundant-declaration.rst
+++ docs/clang-tidy/checks/readability-redundant-declaration.rst
@@ -0,0 +1,17 @@
+.. title:: clang-tidy - readability-redundant-declaration
+
+readability-redundant-declaration
+=
+
+Finds redundant variable declarations.
+
+.. code-block:: c++
+
+  extern int X;
+  extern int X;
+
+becomes
+
+.. code-block:: c++
+
+  extern int X;
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -133,6 +133,7 @@
readability-named-parameter
readability-non-const-parameter
readability-redundant-control-flow
+   readability-redundant-declaration
readability-redundant-smartptr-get
readability-redundant-string-cstr
readability-redundant-string-init
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -111,6 +111,11 @@
   Flags function parameters of a pointer type that could be changed to point to
   a constant type instead.
 
+- New `readability-redundant-declaration
+  `_ check
+
+  Warns about duplicate variable declarations.
+
 Fixed bugs:
 
 - `modernize-make-unique
Index: clang-tidy/readability/RedundantDeclarationCheck.h
===
--- clang-tidy/readability/RedundantDeclarationCheck.h
+++ clang-tidy/readability/RedundantDeclarationCheck.h
@@ -0,0 +1,35 @@
+//===--- RedundantDeclarationCheck.h - clang-tidy*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANT_DECLARATION_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANT_DECLARATION_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Find redundant variable declarations.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-redundant-declaration.html
+class RedundantDeclarationCheck : public ClangTidyCheck {
+public:
+  RedundantDeclarationCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult ) override;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANT_DECLARATION_H
Index: clang-tidy/readability/RedundantDeclarationCheck.cpp
===
--- clang-tidy/readability/RedundantDeclarationCheck.cpp
+++ clang-tidy/readability/RedundantDeclarationCheck.cpp
@@ -0,0 +1,70 @@
+//===--- RedundantDeclarationCheck.cpp - clang-tidy===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. 

Re: [PATCH] D24232: Wbitfiled-constant-conversion : allow ~0 , ~(1<<A), etc

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a subscriber: cfe-commits.
danielmarjamaki added a comment.

ping


https://reviews.llvm.org/D24232



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24656: [clang-tidy] Add check readability-redundant-declaration

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a comment.

> Will be good idea to detect redundant function prototypes.


Yes.

Should that have the same ID though? Is it better to have one 
readability-redundant-declaration or two separate 
readability-redundant-variable-declaration and 
readability-redundant-function-declaration?


Repository:
  rL LLVM

https://reviews.llvm.org/D24656



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24656: [clang-tidy] Add check readability-redundant-declaration

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a comment.

> However, I think this check should be part of Clang diagnostics. GCC has 
> -Wredundant-decls for a long time.


I am not against that.

What is the criteria? When should it be in the compiler and when should it be 
in clang-tidy?

Personally I think it's natural that the compiler has warnings about dangerous 
code. This check is about readability.


Repository:
  rL LLVM

https://reviews.llvm.org/D24656



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24656: [clang-tidy] Add check readability-redundant-declaration

[Daniel Marjamäki via cfe-commits]

danielmarjamaki marked 2 inline comments as done.
danielmarjamaki added a comment.

https://reviews.llvm.org/D24656



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24656: [clang-tidy] Add check readability-redundant-declaration

[Daniel Marjamäki via cfe-commits]

danielmarjamaki updated this revision to Diff 71614.
danielmarjamaki added a comment.

minor fixes


https://reviews.llvm.org/D24656

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tidy/readability/RedundantDeclarationCheck.cpp
  clang-tidy/readability/RedundantDeclarationCheck.h
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-redundant-declaration.rst
  test/clang-tidy/readability-redundant-declaration.cpp

Index: test/clang-tidy/readability-redundant-declaration.cpp
===
--- test/clang-tidy/readability-redundant-declaration.cpp
+++ test/clang-tidy/readability-redundant-declaration.cpp
@@ -0,0 +1,16 @@
+// RUN: %check_clang_tidy %s readability-redundant-declaration %t
+
+extern int Xyz;
+extern int Xyz;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: redundant variable Xyz declaration [readability-redundant-declaration]
+// CHECK-FIXES: {{^}}{{$}}
+
+extern int A;
+extern int A,B;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: redundant variable A declaration
+// CHECK-FIXES: {{^}}extern int A,B;{{$}}
+
+extern int Buf[10];
+extern int Buf[10];
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: redundant variable Buf declaration
+// CHECK-FIXES: {{^}}{{$}}
Index: docs/clang-tidy/checks/readability-redundant-declaration.rst
===
--- docs/clang-tidy/checks/readability-redundant-declaration.rst
+++ docs/clang-tidy/checks/readability-redundant-declaration.rst
@@ -0,0 +1,17 @@
+.. title:: clang-tidy - readability-redundant-declaration
+
+readability-redundant-declaration
+=
+
+Finds redundant variable declarations.
+
+.. code-block:: c++
+
+  extern int X;
+  extern int X;
+
+becomes
+
+.. code-block:: c++
+
+  extern int X;
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -133,6 +133,7 @@
readability-named-parameter
readability-non-const-parameter
readability-redundant-control-flow
+   readability-redundant-declaration
readability-redundant-smartptr-get
readability-redundant-string-cstr
readability-redundant-string-init
Index: clang-tidy/readability/RedundantDeclarationCheck.h
===
--- clang-tidy/readability/RedundantDeclarationCheck.h
+++ clang-tidy/readability/RedundantDeclarationCheck.h
@@ -0,0 +1,35 @@
+//===--- RedundantDeclarationCheck.h - clang-tidy*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANT_DECLARATION_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANT_DECLARATION_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Find redundant variable declarations.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-redundant-declaration.html
+class RedundantDeclarationCheck : public ClangTidyCheck {
+public:
+  RedundantDeclarationCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult ) override;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANT_DECLARATION_H
Index: clang-tidy/readability/RedundantDeclarationCheck.cpp
===
--- clang-tidy/readability/RedundantDeclarationCheck.cpp
+++ clang-tidy/readability/RedundantDeclarationCheck.cpp
@@ -0,0 +1,70 @@
+//===--- RedundantDeclarationCheck.cpp - clang-tidy===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "RedundantDeclarationCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+void RedundantDeclarationCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(varDecl().bind("Decl"), this);
+}
+
+void RedundantDeclarationCheck::check(const MatchFinder::MatchResult ) {
+  const auto *VD = Result.Nodes.getNodeAs("Decl");
+  const 

Re: [PATCH] D24656: [clang-tidy] Add check readability-redundant-declaration

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a comment.

For information, I am testing this on debian packages right now. I will see the 
results next week.



Comment at: clang-tidy/readability/RedundantDeclarationCheck.cpp:22
@@ +21,3 @@
+void RedundantDeclarationCheck::registerMatchers(MatchFinder *Finder) {
+  // FIXME: Add matchers.
+  Finder->addMatcher(varDecl().bind("Decl"), this);

I forgot to remove this FIXME comment, I will remove it.


Comment at: clang-tidy/readability/RedundantDeclarationCheck.cpp:41
@@ +40,3 @@
+
+  // Don't generate fixits for multivariable declarations.
+  bool MultiVar = false;

Imho this is a clumpsy way to see if it's a "multivariable" declaration. Do you 
know if there is a better way?


Comment at: clang-tidy/readability/RedundantDeclarationCheck.cpp:50
@@ +49,3 @@
+
+  if (MultiVar) {
+diag(VD->getLocation(), "redundant variable %0 declaration")

Is there a better way to rewrite this ... I tried something like this without 
success:
```
auto D = diag(..);
if (!MultiVar)
D << FixItHint..;
```



Comment at: clang-tidy/readability/RedundantDeclarationCheck.h:19
@@ +18,3 @@
+
+/// FIXME: Write a short description.
+///

I will fix this FIXME.


Comment at: test/clang-tidy/readability-redundant-declaration.cpp:11
@@ +10,3 @@
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: redundant variable A declaration
+// CHECK-FIXES: {{^}}extern int A,B;{{$}}
+

Ideally this would be changed to "extern int B;". I currently don't fix 
multivariable declarations.


https://reviews.llvm.org/D24656



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D24656: [clang-tidy] Add check readability-redundant-declaration

[Daniel Marjamäki via cfe-commits]

danielmarjamaki created this revision.
danielmarjamaki added a reviewer: alexfh.
danielmarjamaki added a subscriber: cfe-commits.
Herald added subscribers: mgorny, beanz.

This is a new check that warns about redundant variable declarations.

https://reviews.llvm.org/D24656

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tidy/readability/RedundantDeclarationCheck.cpp
  clang-tidy/readability/RedundantDeclarationCheck.h
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-redundant-declaration.rst
  test/clang-tidy/readability-redundant-declaration.cpp

Index: test/clang-tidy/readability-redundant-declaration.cpp
===
--- test/clang-tidy/readability-redundant-declaration.cpp
+++ test/clang-tidy/readability-redundant-declaration.cpp
@@ -0,0 +1,16 @@
+// RUN: %check_clang_tidy %s readability-redundant-declaration %t
+
+extern int Xyz;
+extern int Xyz;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: redundant variable Xyz declaration [readability-redundant-declaration]
+// CHECK-FIXES: {{^}}{{$}}
+
+extern int A;
+extern int A,B;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: redundant variable A declaration
+// CHECK-FIXES: {{^}}extern int A,B;{{$}}
+
+extern int Buf[10];
+extern int Buf[10];
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: redundant variable Buf declaration
+// CHECK-FIXES: {{^}}{{$}}
Index: docs/clang-tidy/checks/readability-redundant-declaration.rst
===
--- docs/clang-tidy/checks/readability-redundant-declaration.rst
+++ docs/clang-tidy/checks/readability-redundant-declaration.rst
@@ -0,0 +1,17 @@
+.. title:: clang-tidy - readability-redundant-declaration
+
+readability-redundant-declaration
+=
+
+Finds redundant variable declarations.
+
+.. code-block:: c++
+
+  extern int X;
+  extern int X;
+
+becomes
+
+.. code-block:: c++
+
+  extern int X;
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -133,6 +133,7 @@
readability-named-parameter
readability-non-const-parameter
readability-redundant-control-flow
+   readability-redundant-declaration
readability-redundant-smartptr-get
readability-redundant-string-cstr
readability-redundant-string-init
Index: clang-tidy/readability/RedundantDeclarationCheck.h
===
--- clang-tidy/readability/RedundantDeclarationCheck.h
+++ clang-tidy/readability/RedundantDeclarationCheck.h
@@ -0,0 +1,35 @@
+//===--- RedundantDeclarationCheck.h - clang-tidy*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANT_DECLARATION_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANT_DECLARATION_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// FIXME: Write a short description.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-redundant-declaration.html
+class RedundantDeclarationCheck : public ClangTidyCheck {
+public:
+  RedundantDeclarationCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult ) override;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANT_DECLARATION_H
Index: clang-tidy/readability/RedundantDeclarationCheck.cpp
===
--- clang-tidy/readability/RedundantDeclarationCheck.cpp
+++ clang-tidy/readability/RedundantDeclarationCheck.cpp
@@ -0,0 +1,65 @@
+//===--- RedundantDeclarationCheck.cpp - clang-tidy===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "RedundantDeclarationCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+void RedundantDeclarationCheck::registerMatchers(MatchFinder *Finder) {
+  // FIXME: Add matchers.
+  

Re: [PATCH] D21134: clang-tidy: new check readability-misplaced-array-index

[Daniel Marjamäki via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL281206: [clang-tidy] readability-misplaced-array-index: add 
new check that warns when… (authored by danielmarjamaki).

Changed prior to commit:
  https://reviews.llvm.org/D21134?vs=69558=70994#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D21134

Files:
  clang-tools-extra/trunk/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/trunk/clang-tidy/readability/MisplacedArrayIndexCheck.cpp
  clang-tools-extra/trunk/clang-tidy/readability/MisplacedArrayIndexCheck.h
  clang-tools-extra/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/trunk/docs/ReleaseNotes.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/trunk/docs/clang-tidy/checks/readability-misplaced-array-index.rst
  clang-tools-extra/trunk/test/clang-tidy/readability-misplaced-array-index.cpp

Index: clang-tools-extra/trunk/test/clang-tidy/readability-misplaced-array-index.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/readability-misplaced-array-index.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/readability-misplaced-array-index.cpp
@@ -0,0 +1,34 @@
+// RUN: %check_clang_tidy %s readability-misplaced-array-index %t
+
+#define ABC  "abc"
+
+struct XY { int *X; int *Y; };
+
+void dostuff(int);
+
+void unusualSyntax(int *P1, struct XY *P2) {
+  10[P1] = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: confusing array subscript expression, usually the index is inside the []
+  // CHECK-FIXES: P1[10] = 0;
+
+  10[P2->X] = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: confusing array subscript expression
+  // CHECK-FIXES: P2->X[10] = 0;
+
+  dostuff(1["abc"]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: confusing array subscript expression
+  // CHECK-FIXES:  dostuff("abc"[1]);
+
+  dostuff(1[ABC]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: confusing array subscript expression
+  // CHECK-FIXES:  dostuff(ABC[1]);
+
+  dostuff(0[0 + ABC]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: confusing array subscript expression
+  // CHECK-FIXES:  dostuff(0[0 + ABC]);
+  // No fixit. Probably the code should be ABC[0]
+}
+
+void normalSyntax(int *X) {
+  X[10] = 0;
+}
Index: clang-tools-extra/trunk/clang-tidy/readability/MisplacedArrayIndexCheck.h
===
--- clang-tools-extra/trunk/clang-tidy/readability/MisplacedArrayIndexCheck.h
+++ clang-tools-extra/trunk/clang-tidy/readability/MisplacedArrayIndexCheck.h
@@ -0,0 +1,36 @@
+//===--- MisplacedArrayIndexCheck.h - clang-tidy-*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_MISPLACED_ARRAY_INDEX_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_MISPLACED_ARRAY_INDEX_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Warn about unusual array index syntax (`index[array]` instead of
+/// `array[index]`).
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-misplaced-array-index.html
+class MisplacedArrayIndexCheck : public ClangTidyCheck {
+public:
+  MisplacedArrayIndexCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult ) override;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_MISPLACED_ARRAY_INDEX_H
Index: clang-tools-extra/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp
===
--- clang-tools-extra/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp
+++ clang-tools-extra/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -19,6 +19,7 @@
 #include "IdentifierNamingCheck.h"
 #include "ImplicitBoolCastCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"
+#include "MisplacedArrayIndexCheck.h"
 #include "NamedParameterCheck.h"
 #include "NonConstParameterCheck.h"
 #include "RedundantControlFlowCheck.h"
@@ -54,6 +55,8 @@
 "readability-implicit-bool-cast");
 CheckFactories.registerCheck(
 "readability-inconsistent-declaration-parameter-name");
+CheckFactories.registerCheck(
+"readability-misplaced-array-index");
 CheckFactories.registerCheck(
 "readability-static-definition-in-anonymous-namespace");
 CheckFactories.registerCheck(
Index: clang-tools-extra/trunk/clang-tidy/readability/CMakeLists.txt

Re: [PATCH] D24238: StaticAnalyzer CastToStruct : No memory corruption when casting array to struct

[Daniel Marjamäki via cfe-commits]

danielmarjamaki abandoned this revision.
danielmarjamaki added a comment.

hmm.. I don't actually care much about this specific check at the moment.

I saw other false positives (unreachable code) and thought that this check made 
the analyzer think there was corrupted memory.

Now I can't reproduce my other false positives.

I'll close this for now. Unless I get those other FP again I don't care about 
this.


https://reviews.llvm.org/D24238



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24238: StaticAnalyzer CastToStruct : No memory corruption when casting array to struct

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a comment.

If the -fno-strict-aliasing would fix this warning then it would be OK.

If you are telling me that this CastToStruct check is about alignment or 
endianness then I think the message is highly misleading. We should rewrite the 
message.

In general, using char instead of short/int does not prevent 
alignment/endianness problems as far as I see. You can still have just as many 
such problems even though array is char.

I am not sure why they did not use char here. But on their platform, 
sizeof(char)==sizeof(short)==sizeof(int)==1. It does not matter much if a 
typedef uses char or int.


https://reviews.llvm.org/D24238



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D21134: clang-tidy: new check readability-misplaced-array-index

[Daniel Marjamäki via cfe-commits]

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

https://reviews.llvm.org/D21134



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D21134: clang-tidy: new check readability-misplaced-array-index

[Daniel Marjamäki via cfe-commits]

danielmarjamaki updated this revision to Diff 69558.
danielmarjamaki marked 2 inline comments as done.
danielmarjamaki added a comment.

fix review comments


https://reviews.llvm.org/D21134

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/MisplacedArrayIndexCheck.cpp
  clang-tidy/readability/MisplacedArrayIndexCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-misplaced-array-index.rst
  test/clang-tidy/readability-misplaced-array-index.cpp

Index: test/clang-tidy/readability-misplaced-array-index.cpp
===
--- test/clang-tidy/readability-misplaced-array-index.cpp
+++ test/clang-tidy/readability-misplaced-array-index.cpp
@@ -0,0 +1,34 @@
+// RUN: %check_clang_tidy %s readability-misplaced-array-index %t
+
+#define ABC  "abc"
+
+struct XY { int *X; int *Y; };
+
+void dostuff(int);
+
+void unusualSyntax(int *P1, struct XY *P2) {
+  10[P1] = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: confusing array subscript expression, usually the index is inside the []
+  // CHECK-FIXES: P1[10] = 0;
+
+  10[P2->X] = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: confusing array subscript expression
+  // CHECK-FIXES: P2->X[10] = 0;
+
+  dostuff(1["abc"]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: confusing array subscript expression
+  // CHECK-FIXES:  dostuff("abc"[1]);
+
+  dostuff(1[ABC]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: confusing array subscript expression
+  // CHECK-FIXES:  dostuff(ABC[1]);
+
+  dostuff(0[0 + ABC]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: confusing array subscript expression
+  // CHECK-FIXES:  dostuff(0[0 + ABC]);
+  // No fixit. Probably the code should be ABC[0]
+}
+
+void normalSyntax(int *X) {
+  X[10] = 0;
+}
Index: docs/clang-tidy/checks/readability-misplaced-array-index.rst
===
--- docs/clang-tidy/checks/readability-misplaced-array-index.rst
+++ docs/clang-tidy/checks/readability-misplaced-array-index.rst
@@ -0,0 +1,27 @@
+.. title:: clang-tidy - readability-misplaced-array-index
+
+readability-misplaced-array-index
+=
+
+This check warns for unusual array index syntax.
+
+The following code has unusual array index syntax:
+
+.. code-block:: c++
+
+  void f(int *X, int Y) {
+Y[X] = 0;
+  }
+
+becomes
+
+.. code-block:: c++
+
+  void f(int *X, int Y) {
+X[Y] = 0;
+  }
+
+The check warns about such unusual syntax for readability reasons:
+ * There are programmers that are not familiar with this unusual syntax.
+ * It is possible that variables are mixed up.
+
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -127,6 +127,7 @@
readability-identifier-naming
readability-implicit-bool-cast
readability-inconsistent-declaration-parameter-name
+   readability-misplaced-array-index
readability-named-parameter
readability-non-const-parameter
readability-redundant-control-flow
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -85,6 +85,11 @@
   Warns about the performance overhead arising from concatenating strings using
   the ``operator+``, instead of ``operator+=``.
 
+- New `readability-misplaced-array-index
+  `_ check
+
+  Warns when there is array index before the [] instead of inside it.
+
 - New `readability-non-const-parameter
   `_ check
 
Index: clang-tidy/readability/ReadabilityTidyModule.cpp
===
--- clang-tidy/readability/ReadabilityTidyModule.cpp
+++ clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -19,6 +19,7 @@
 #include "IdentifierNamingCheck.h"
 #include "ImplicitBoolCastCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"
+#include "MisplacedArrayIndexCheck.h"
 #include "NamedParameterCheck.h"
 #include "NonConstParameterCheck.h"
 #include "RedundantControlFlowCheck.h"
@@ -54,6 +55,8 @@
 "readability-implicit-bool-cast");
 CheckFactories.registerCheck(
 "readability-inconsistent-declaration-parameter-name");
+CheckFactories.registerCheck(
+"readability-misplaced-array-index");
 CheckFactories.registerCheck(
 "readability-static-definition-in-anonymous-namespace");
 CheckFactories.registerCheck(
Index: clang-tidy/readability/MisplacedArrayIndexCheck.h
===
--- clang-tidy/readability/MisplacedArrayIndexCheck.h
+++ 

Re: [PATCH] D21134: clang-tidy: new check readability-misplaced-array-index

[Daniel Marjamäki via cfe-commits]

danielmarjamaki marked 6 inline comments as done.


Comment at: clang-tidy/readability/MisplacedArrayIndexCheck.cpp:57
@@ +56,2 @@
+} // namespace tidy
+} // namespace clang

I removed hasMacroId() and use fixit::getText(). The replacements look good now.


Comment at: docs/clang-tidy/checks/readability-misplaced-array-index.rst:13
@@ +12,3 @@
+  void f(int *x, int y) {
+y[x] = 0;
+  }

ok thanks same mistake I've done before. Should I start using uppercase 
variable names from now on?


https://reviews.llvm.org/D21134



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D21134: clang-tidy: new check readability-misplaced-array-index

[Daniel Marjamäki via cfe-commits]

danielmarjamaki updated this revision to Diff 69113.
danielmarjamaki added a comment.

fixed review comments


https://reviews.llvm.org/D21134

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/MisplacedArrayIndexCheck.cpp
  clang-tidy/readability/MisplacedArrayIndexCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-misplaced-array-index.rst
  test/clang-tidy/readability-misplaced-array-index.cpp

Index: test/clang-tidy/readability-misplaced-array-index.cpp
===
--- test/clang-tidy/readability-misplaced-array-index.cpp
+++ test/clang-tidy/readability-misplaced-array-index.cpp
@@ -0,0 +1,36 @@
+// RUN: %check_clang_tidy %s readability-misplaced-array-index %t
+
+#define ABC  "abc"
+
+struct XY { int *x; int *y; };
+
+void dostuff(int);
+
+void unusualSyntax(int *x, struct XY *xy)
+{
+  10[x] = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: confusing array subscript expression, usually the index is inside the []
+  // CHECK-FIXES: x[10] = 0;
+
+  10[xy->x] = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: confusing array subscript expression
+  // CHECK-FIXES: xy->x[10] = 0;
+
+  dostuff(1["abc"]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: confusing array subscript expression
+  // CHECK-FIXES:  dostuff("abc"[1]);
+
+  dostuff(1[ABC]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: confusing array subscript expression
+  // CHECK-FIXES:  dostuff(ABC[1]);
+
+  dostuff(0[0 + ABC]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: confusing array subscript expression
+  // CHECK-FIXES:  dostuff(0[0 + ABC]);
+  // No fixit. Probably the code should be ABC[0]
+}
+
+void normalSyntax(int *x)
+{
+  x[10] = 0;
+}
Index: docs/clang-tidy/checks/readability-misplaced-array-index.rst
===
--- docs/clang-tidy/checks/readability-misplaced-array-index.rst
+++ docs/clang-tidy/checks/readability-misplaced-array-index.rst
@@ -0,0 +1,27 @@
+.. title:: clang-tidy - readability-misplaced-array-index
+
+readability-misplaced-array-index
+=
+
+This check warns for unusual array index syntax.
+
+The following code has unusual array index syntax:
+
+.. code:: c++
+
+  void f(int *x, int y) {
+y[x] = 0;
+  }
+
+becomes
+
+.. code:: c++
+
+  void f(int *x, int y) {
+x[y] = 0;
+  }
+
+The check warns about such unusual syntax for readability reasons:
+ * There are programmers that are not familiar with this unusual syntax.
+ * It is possible that variables are mixed up.
+
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -127,6 +127,7 @@
readability-identifier-naming
readability-implicit-bool-cast
readability-inconsistent-declaration-parameter-name
+   readability-misplaced-array-index
readability-named-parameter
readability-non-const-parameter
readability-redundant-control-flow
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -85,6 +85,11 @@
   Warns about the performance overhead arising from concatenating strings using
   the ``operator+``, instead of ``operator+=``.
 
+- New `readability-misplaced-array-index
+  `_ check
+
+  Warns when there is array index before the [] instead of inside it.
+
 - New `readability-non-const-parameter
   `_ check
 
Index: clang-tidy/readability/ReadabilityTidyModule.cpp
===
--- clang-tidy/readability/ReadabilityTidyModule.cpp
+++ clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -19,6 +19,7 @@
 #include "IdentifierNamingCheck.h"
 #include "ImplicitBoolCastCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"
+#include "MisplacedArrayIndexCheck.h"
 #include "NamedParameterCheck.h"
 #include "NonConstParameterCheck.h"
 #include "RedundantControlFlowCheck.h"
@@ -54,6 +55,8 @@
 "readability-implicit-bool-cast");
 CheckFactories.registerCheck(
 "readability-inconsistent-declaration-parameter-name");
+CheckFactories.registerCheck(
+"readability-misplaced-array-index");
 CheckFactories.registerCheck(
 "readability-static-definition-in-anonymous-namespace");
 CheckFactories.registerCheck(
Index: clang-tidy/readability/MisplacedArrayIndexCheck.h
===
--- clang-tidy/readability/MisplacedArrayIndexCheck.h
+++ clang-tidy/readability/MisplacedArrayIndexCheck.h
@@ -0,0 +1,36 @@
+//===--- 

Re: [PATCH] D15332: new clang-tidy checker readability-non-const-parameter

[Daniel Marjamäki via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL279507: [clang-tidy] readability-non-const-parameter: add 
new check that warns when… (authored by danielmarjamaki).

Changed prior to commit:
  https://reviews.llvm.org/D15332?vs=68531=68963#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D15332

Files:
  clang-tools-extra/trunk/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/trunk/clang-tidy/readability/NonConstParameterCheck.cpp
  clang-tools-extra/trunk/clang-tidy/readability/NonConstParameterCheck.h
  clang-tools-extra/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/trunk/docs/ReleaseNotes.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/trunk/docs/clang-tidy/checks/readability-non-const-parameter.rst
  clang-tools-extra/trunk/test/clang-tidy/readability-non-const-parameter.cpp

Index: clang-tools-extra/trunk/clang-tidy/readability/NonConstParameterCheck.h
===
--- clang-tools-extra/trunk/clang-tidy/readability/NonConstParameterCheck.h
+++ clang-tools-extra/trunk/clang-tidy/readability/NonConstParameterCheck.h
@@ -0,0 +1,64 @@
+//===--- NonConstParameterCheck.h - clang-tidy---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_NON_CONST_PARAMETER_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_NON_CONST_PARAMETER_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Warn when a pointer function parameter can be const.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-non-const-parameter.html
+class NonConstParameterCheck : public ClangTidyCheck {
+public:
+  NonConstParameterCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult ) override;
+  void onEndOfTranslationUnit() override;
+
+private:
+  /// Parameter info.
+  struct ParmInfo {
+/// Is function parameter referenced?
+bool IsReferenced;
+
+/// Can function parameter be const?
+bool CanBeConst;
+  };
+
+  /// Track all nonconst integer/float parameters.
+  std::map Parameters;
+
+  /// Add function parameter.
+  void addParm(const ParmVarDecl *Parm);
+
+  /// Set IsReferenced.
+  void setReferenced(const DeclRefExpr *Ref);
+
+  /// Set CanNotBeConst.
+  /// Visits sub expressions recursively. If a DeclRefExpr is found
+  /// and CanNotBeConst is true the Parameter is marked as not-const.
+  /// The CanNotBeConst is updated as sub expressions are visited.
+  void markCanNotBeConst(const Expr *E, bool CanNotBeConst);
+
+  /// Diagnose non const parameters.
+  void diagnoseNonConstParameters();
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_NON_CONST_PARAMETER_H
Index: clang-tools-extra/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp
===
--- clang-tools-extra/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp
+++ clang-tools-extra/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -20,6 +20,7 @@
 #include "ImplicitBoolCastCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"
 #include "NamedParameterCheck.h"
+#include "NonConstParameterCheck.h"
 #include "RedundantControlFlowCheck.h"
 #include "RedundantSmartptrGetCheck.h"
 #include "RedundantStringCStrCheck.h"
@@ -57,6 +58,8 @@
 "readability-static-definition-in-anonymous-namespace");
 CheckFactories.registerCheck(
 "readability-named-parameter");
+CheckFactories.registerCheck(
+"readability-non-const-parameter");
 CheckFactories.registerCheck(
 "readability-redundant-control-flow");
 CheckFactories.registerCheck(
Index: clang-tools-extra/trunk/clang-tidy/readability/NonConstParameterCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/readability/NonConstParameterCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/readability/NonConstParameterCheck.cpp
@@ -0,0 +1,214 @@
+//===--- NonConstParameterCheck.cpp - clang-tidy---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include 

Re: [PATCH] D15332: new clang-tidy checker readability-non-const-parameter

[Daniel Marjamäki via cfe-commits]

danielmarjamaki updated this revision to Diff 68531.
danielmarjamaki added a comment.

Fixed review comments about formatting in doc


https://reviews.llvm.org/D15332

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/NonConstParameterCheck.cpp
  clang-tidy/readability/NonConstParameterCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-non-const-parameter.rst
  test/clang-tidy/readability-non-const-parameter.cpp

Index: test/clang-tidy/readability-non-const-parameter.cpp
===
--- test/clang-tidy/readability-non-const-parameter.cpp
+++ test/clang-tidy/readability-non-const-parameter.cpp
@@ -0,0 +1,279 @@
+// RUN: %check_clang_tidy %s readability-non-const-parameter %t
+
+// Currently the checker only warns about pointer arguments.
+//
+// It can be defined both that the data is const and that the pointer is const,
+// the checker only checks if the data can be const-specified.
+//
+// It does not warn about pointers to records or function pointers.
+
+// Some external function where first argument is nonconst and second is const.
+char *strcpy1(char *dest, const char *src);
+unsigned my_strcpy(char *buf, const char *s);
+unsigned my_strlen(const char *buf);
+
+// CHECK-MESSAGES: :[[@LINE+1]]:29: warning: pointer parameter 'last' can be pointer to const [readability-non-const-parameter]
+void warn1(int *first, int *last) {
+  // CHECK-FIXES: {{^}}void warn1(int *first, const int *last) {{{$}}
+  *first = 0;
+  if (first < last) {
+  } // <- last can be const
+}
+
+// TODO: warning should be written here
+void warn2(char *p) {
+  char buf[10];
+  strcpy1(buf, p);
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:19: warning: pointer parameter 'p' can be
+void assign1(int *p) {
+  // CHECK-FIXES: {{^}}void assign1(const int *p) {{{$}}
+  const int *q;
+  q = p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:19: warning: pointer parameter 'p' can be
+void assign2(int *p) {
+  // CHECK-FIXES: {{^}}void assign2(const int *p) {{{$}}
+  const int *q;
+  q = p + 1;
+}
+
+void assign3(int *p) {
+  *p = 0;
+}
+
+void assign4(int *p) {
+  *p += 2;
+}
+
+void assign5(char *p) {
+  p[0] = 0;
+}
+
+void assign6(int *p) {
+  int *q;
+  q = p++;
+}
+
+void assign7(char *p) {
+  char *a, *b;
+  a = b = p;
+}
+
+void assign8(char *a, char *b) {
+  char *x;
+  x = (a ? a : b);
+}
+
+void assign9(unsigned char *str, const unsigned int i) {
+  unsigned char *p;
+  for (p = str + i; *p;) {
+  }
+}
+
+void assign10(int *buf) {
+  int i, *p;
+  for (i = 0, p = buf; i < 10; i++, p++) {
+*p = 1;
+  }
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:17: warning: pointer parameter 'p' can be
+void init1(int *p) {
+  // CHECK-FIXES: {{^}}void init1(const int *p) {{{$}}
+  const int *q = p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:17: warning: pointer parameter 'p' can be
+void init2(int *p) {
+  // CHECK-FIXES: {{^}}void init2(const int *p) {{{$}}
+  const int *q = p + 1;
+}
+
+void init3(int *p) {
+  int *q = p;
+}
+
+void init4(float *p) {
+  int *q = (int *)p;
+}
+
+void init5(int *p) {
+  int *i = p ? p : 0;
+}
+
+void init6(int *p) {
+  int *a[] = {p, p, 0};
+}
+
+void init7(int *p, int x) {
+  for (int *q = p + x - 1; 0; q++)
+;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:18: warning: pointer parameter 'p' can be
+int return1(int *p) {
+  // CHECK-FIXES: {{^}}int return1(const int *p) {{{$}}
+  return *p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:25: warning: pointer parameter 'p' can be
+const int *return2(int *p) {
+  // CHECK-FIXES: {{^}}const int *return2(const int *p) {{{$}}
+  return p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:25: warning: pointer parameter 'p' can be
+const int *return3(int *p) {
+  // CHECK-FIXES: {{^}}const int *return3(const int *p) {{{$}}
+  return p + 1;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:27: warning: pointer parameter 'p' can be
+const char *return4(char *p) {
+  // CHECK-FIXES: {{^}}const char *return4(const char *p) {{{$}}
+  return p ? p : "";
+}
+
+char *return5(char *s) {
+  return s;
+}
+
+char *return6(char *s) {
+  return s + 1;
+}
+
+char *return7(char *a, char *b) {
+  return a ? a : b;
+}
+
+char return8(int *p) {
+  return ++(*p);
+}
+
+void dontwarn1(int *p) {
+  ++(*p);
+}
+
+void dontwarn2(int *p) {
+  (*p)++;
+}
+
+int dontwarn3(_Atomic(int) * p) {
+  return *p;
+}
+
+void callFunction1(char *p) {
+  strcpy1(p, "abc");
+}
+
+void callFunction2(char *p) {
+  strcpy1([0], "abc");
+}
+
+void callFunction3(char *p) {
+  strcpy1(p + 2, "abc");
+}
+
+char *callFunction4(char *p) {
+  return strcpy1(p, "abc");
+}
+
+unsigned callFunction5(char *buf) {
+  unsigned len = my_strlen(buf);
+  return len + my_strcpy(buf, "abc");
+}
+
+void f6(int **p);
+void callFunction6(int *p) { f6(); }
+
+typedef union { void *v; } t;
+void f7(t obj);
+void callFunction7(int *p) {
+  f7((t){p});
+}
+
+void f8(int );
+void callFunction8(int *p) {
+  f8(*p);
+}
+

Re: [PATCH] D15332: new clang-tidy checker readability-non-const-parameter

[Daniel Marjamäki via cfe-commits]

danielmarjamaki updated this revision to Diff 68528.
danielmarjamaki marked 2 inline comments as done.
danielmarjamaki added a comment.

Fixed review comments


https://reviews.llvm.org/D15332

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/NonConstParameterCheck.cpp
  clang-tidy/readability/NonConstParameterCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-non-const-parameter.rst
  test/clang-tidy/readability-non-const-parameter.cpp

Index: test/clang-tidy/readability-non-const-parameter.cpp
===
--- test/clang-tidy/readability-non-const-parameter.cpp
+++ test/clang-tidy/readability-non-const-parameter.cpp
@@ -0,0 +1,279 @@
+// RUN: %check_clang_tidy %s readability-non-const-parameter %t
+
+// Currently the checker only warns about pointer arguments.
+//
+// It can be defined both that the data is const and that the pointer is const,
+// the checker only checks if the data can be const-specified.
+//
+// It does not warn about pointers to records or function pointers.
+
+// Some external function where first argument is nonconst and second is const.
+char *strcpy1(char *dest, const char *src);
+unsigned my_strcpy(char *buf, const char *s);
+unsigned my_strlen(const char *buf);
+
+// CHECK-MESSAGES: :[[@LINE+1]]:29: warning: pointer parameter 'last' can be pointer to const [readability-non-const-parameter]
+void warn1(int *first, int *last) {
+  // CHECK-FIXES: {{^}}void warn1(int *first, const int *last) {{{$}}
+  *first = 0;
+  if (first < last) {
+  } // <- last can be const
+}
+
+// TODO: warning should be written here
+void warn2(char *p) {
+  char buf[10];
+  strcpy1(buf, p);
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:19: warning: pointer parameter 'p' can be
+void assign1(int *p) {
+  // CHECK-FIXES: {{^}}void assign1(const int *p) {{{$}}
+  const int *q;
+  q = p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:19: warning: pointer parameter 'p' can be
+void assign2(int *p) {
+  // CHECK-FIXES: {{^}}void assign2(const int *p) {{{$}}
+  const int *q;
+  q = p + 1;
+}
+
+void assign3(int *p) {
+  *p = 0;
+}
+
+void assign4(int *p) {
+  *p += 2;
+}
+
+void assign5(char *p) {
+  p[0] = 0;
+}
+
+void assign6(int *p) {
+  int *q;
+  q = p++;
+}
+
+void assign7(char *p) {
+  char *a, *b;
+  a = b = p;
+}
+
+void assign8(char *a, char *b) {
+  char *x;
+  x = (a ? a : b);
+}
+
+void assign9(unsigned char *str, const unsigned int i) {
+  unsigned char *p;
+  for (p = str + i; *p;) {
+  }
+}
+
+void assign10(int *buf) {
+  int i, *p;
+  for (i = 0, p = buf; i < 10; i++, p++) {
+*p = 1;
+  }
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:17: warning: pointer parameter 'p' can be
+void init1(int *p) {
+  // CHECK-FIXES: {{^}}void init1(const int *p) {{{$}}
+  const int *q = p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:17: warning: pointer parameter 'p' can be
+void init2(int *p) {
+  // CHECK-FIXES: {{^}}void init2(const int *p) {{{$}}
+  const int *q = p + 1;
+}
+
+void init3(int *p) {
+  int *q = p;
+}
+
+void init4(float *p) {
+  int *q = (int *)p;
+}
+
+void init5(int *p) {
+  int *i = p ? p : 0;
+}
+
+void init6(int *p) {
+  int *a[] = {p, p, 0};
+}
+
+void init7(int *p, int x) {
+  for (int *q = p + x - 1; 0; q++)
+;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:18: warning: pointer parameter 'p' can be
+int return1(int *p) {
+  // CHECK-FIXES: {{^}}int return1(const int *p) {{{$}}
+  return *p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:25: warning: pointer parameter 'p' can be
+const int *return2(int *p) {
+  // CHECK-FIXES: {{^}}const int *return2(const int *p) {{{$}}
+  return p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:25: warning: pointer parameter 'p' can be
+const int *return3(int *p) {
+  // CHECK-FIXES: {{^}}const int *return3(const int *p) {{{$}}
+  return p + 1;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:27: warning: pointer parameter 'p' can be
+const char *return4(char *p) {
+  // CHECK-FIXES: {{^}}const char *return4(const char *p) {{{$}}
+  return p ? p : "";
+}
+
+char *return5(char *s) {
+  return s;
+}
+
+char *return6(char *s) {
+  return s + 1;
+}
+
+char *return7(char *a, char *b) {
+  return a ? a : b;
+}
+
+char return8(int *p) {
+  return ++(*p);
+}
+
+void dontwarn1(int *p) {
+  ++(*p);
+}
+
+void dontwarn2(int *p) {
+  (*p)++;
+}
+
+int dontwarn3(_Atomic(int) * p) {
+  return *p;
+}
+
+void callFunction1(char *p) {
+  strcpy1(p, "abc");
+}
+
+void callFunction2(char *p) {
+  strcpy1([0], "abc");
+}
+
+void callFunction3(char *p) {
+  strcpy1(p + 2, "abc");
+}
+
+char *callFunction4(char *p) {
+  return strcpy1(p, "abc");
+}
+
+unsigned callFunction5(char *buf) {
+  unsigned len = my_strlen(buf);
+  return len + my_strcpy(buf, "abc");
+}
+
+void f6(int **p);
+void callFunction6(int *p) { f6(); }
+
+typedef union { void *v; } t;
+void f7(t obj);
+void callFunction7(int *p) {
+  f7((t){p});
+}
+
+void f8(int );
+void 

Re: [PATCH] D15332: new clang-tidy checker readability-non-const-parameter

[Daniel Marjamäki via cfe-commits]

danielmarjamaki marked 6 inline comments as done.


Comment at: clang-tidy/readability/NonConstParameterCheck.cpp:95-98
@@ +94,6 @@
+const QualType T = VD->getType();
+if (T->isPointerType() && !T->getPointeeType().isConstQualified())
+  markCanNotBeConst(VD->getInit(), true);
+else if (T->isArrayType())
+  markCanNotBeConst(VD->getInit(), true);
+  }

alexfh wrote:
> danielmarjamaki wrote:
> > Prazek wrote:
> > > This looks like it could be in the same if.
> > Yes it could. But would it make the code more or less readable? It wouldn't 
> > be a 1-line condition anymore then.
> I also think that it makes sense to merge the conditions. The problem with 
> the current code is that it is suspicious ("Why is the same action is done in 
> two branches? Is it a bug?"). One line condition vs two lines seems secondary 
> in this case.
ok


Comment at: clang-tidy/readability/NonConstParameterCheck.cpp:103
@@ +102,3 @@
+void NonConstParameterCheck::addParm(const ParmVarDecl *Parm) {
+  // Only add nonconst integer/float pointer parameters.
+  const QualType T = Parm->getType();

alexfh wrote:
> This seems too strict. What about other primitive types? 
I am not sure which type you are talking about. As far as I see we're writing 
warnings about bool,char,short,int,long,long long,float,double,long double,enum 
pointers.

I have intentionally avoided records now to start with. It should be added, but 
we need to be more careful when we do it.



Comment at: test/clang-tidy/readability-non-const-parameter.cpp:210
@@ +209,3 @@
+// CHECK-MESSAGES: :[[@LINE+1]]:27: warning: pointer parameter 'p' can be
+int functionpointer2(int *p)
+{

alexfh wrote:
> Put braces on the previous line, please. A few other instances below.
sorry .. of course I should run clang-format on this.


https://reviews.llvm.org/D15332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D15332: new clang-tidy checker readability-non-const-parameter

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added inline comments.


Comment at: clang-tidy/readability/NonConstParameterCheck.cpp:95-98
@@ +94,6 @@
+const QualType T = VD->getType();
+if (T->isPointerType() && !T->getPointeeType().isConstQualified())
+  markCanNotBeConst(VD->getInit(), true);
+else if (T->isArrayType())
+  markCanNotBeConst(VD->getInit(), true);
+  }

Prazek wrote:
> This looks like it could be in the same if.
Yes it could. But would it make the code more or less readable? It wouldn't be 
a 1-line condition anymore then.


https://reviews.llvm.org/D15332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D13126: New static analyzer checker for loss of sign/precision

[Daniel Marjamäki via cfe-commits]

danielmarjamaki marked 3 inline comments as done.


Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:67
@@ +66,3 @@
+  const Stmt *Parent = PM.getParent(Cast);
+  if (!Parent)
+return;

I get assertion failure then when running this test:  
Analysis/misc-ps-region-store.cpp

Command that fails:

/home/danielm/llvm/build/./bin/clang -cc1 -internal-isystem 
/home/danielm/llvm/build/bin/../lib/clang/4.0.0/include -nostdsysteminc -triple 
i386-apple-darwin9 -analyze 
-analyzer-checker=core,alpha.core,debug.ExprInspection -analyzer-store=region 
-verify -fblocks -analyzer-opt-analyze-nested-blocks 
/home/danielm/llvm/tools/clang/test/Analysis/misc-ps-region-store.cpp 
-fexceptions -fcxx-exceptions -Wno-tautological-undefined-compare



https://reviews.llvm.org/D13126



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D13126: New static analyzer checker for loss of sign/precision

[Daniel Marjamäki via cfe-commits]

danielmarjamaki updated this revision to Diff 67817.
danielmarjamaki added a comment.

fixed review comments


https://reviews.llvm.org/D13126

Files:
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/CMakeLists.txt
  lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
  test/Analysis/conversion.c

Index: test/Analysis/conversion.c
===
--- test/Analysis/conversion.c
+++ test/Analysis/conversion.c
@@ -0,0 +1,125 @@
+// RUN: %clang_cc1 -Wno-conversion -analyze -analyzer-checker=core,alpha.core.Conversion -verify %s
+
+unsigned char U8;
+signed char S8;
+
+void assign(unsigned U, signed S) {
+  if (S < -10)
+U8 = S; // expected-warning {{Loss of sign in implicit conversion}}
+  if (U > 300)
+S8 = U; // expected-warning {{Loss of precision in implicit conversion}}
+  if (S > 10)
+U8 = S;
+  if (U < 200)
+S8 = U;
+}
+
+void init1() {
+  long long A = 1LL << 60;
+  short X = A; // expected-warning {{Loss of precision in implicit conversion}}
+}
+
+void relational(unsigned U, signed S) {
+  if (S > 10) {
+if (U < S) {
+}
+  }
+  if (S < -10) {
+if (U < S) { // expected-warning {{Loss of sign in implicit conversion}}
+}
+  }
+}
+
+void multiplication(unsigned U, signed S) {
+  if (S > 5)
+S = U * S;
+  if (S < -10)
+S = U * S; // expected-warning {{Loss of sign}}
+}
+
+void division(unsigned U, signed S) {
+  if (S > 5)
+S = U / S;
+  if (S < -10)
+S = U / S; // expected-warning {{Loss of sign}}
+}
+
+void dontwarn1(unsigned U, signed S) {
+  U8 = S; // It might be known that S is always 0x00-0xff.
+  S8 = U; // It might be known that U is always 0x00-0xff.
+
+  U8 = -1;  // Explicit conversion.
+  S8 = ~0U; // Explicit conversion.
+  if (U > 300)
+U8 &= U; // No loss of precision since there is &=.
+}
+
+void dontwarn2(unsigned int U) {
+  if (U <= 4294967295) {
+  }
+  if (U <= (2147483647 * 2U + 1U)) {
+  }
+}
+
+void dontwarn3(int X) {
+  S8 = X ? 'a' : 'b';
+}
+
+// don't warn for macros
+#define DOSTUFF ({ unsigned X = 1000; U8 = X; })
+void dontwarn4() {
+  DOSTUFF;
+}
+
+// don't warn for calculations
+// seen some fp. For instance:  c2 = (c2 >= 'A' && c2 <= 'Z') ? c2 - 'A' + 'a' : c2;
+// there is a todo in the checker to handle calculations
+void dontwarn5() {
+  signed S = -32;
+  U8 = S + 10;
+}
+
+
+// false positives..
+
+int isascii(int c);
+void falsePositive1() {
+  char kb2[5];
+  int X = 1000;
+  if (isascii(X)) {
+// FIXME: should not warn here:
+kb2[0] = X; // expected-warning {{Loss of precision}}
+  }
+}
+
+
+typedef struct FILE {} FILE; int getc(FILE *stream);
+# define EOF (-1)
+char reply_string[8192];
+FILE *cin;
+extern int dostuff (void);
+int falsePositive2() {
+  int c, n;
+  int dig;
+  char *cp = reply_string;
+  int pflag = 0;
+  int code;
+
+  for (;;) {
+dig = n = code = 0;
+while ((c = getc(cin)) != '\n') {
+  if (dig < 4 && dostuff())
+code = code * 10 + (c - '0');
+  if (!pflag && code == 227)
+pflag = 1;
+  if (n == 0)
+n = c;
+  if (c == EOF)
+return(4);
+  if (cp < _string[sizeof(reply_string) - 1])
+// FIXME: should not warn here:
+*cp++ = c; // expected-warning {{Loss of precision}}
+}
+  }
+}
+
Index: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
+++ lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
@@ -0,0 +1,192 @@
+//=== ConversionChecker.cpp -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// Check that there is no loss of sign/precision in assignments, comparisons
+// and multiplications.
+//
+// ConversionChecker uses path sensitive analysis to determine possible values
+// of expressions. A warning is reported when:
+// * a negative value is implicitly converted to an unsigned value in an
+//   assignment, comparison or multiplication.
+// * assignment / initialization when source value is greater than the max
+//   value of target
+//
+// Many compilers and tools have similar checks that are based on semantic
+// analysis. Those checks are sound but have poor precision. ConversionChecker
+// is an alternative to those checks.
+//
+//===--===//
+#include "ClangSACheckers.h"
+#include "clang/AST/ParentMap.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+
+using namespace clang;
+using 

Re: [PATCH] D15332: new clang-tidy checker readability-non-const-parameter

[Daniel Marjamäki via cfe-commits]

danielmarjamaki marked 2 inline comments as done.


Comment at: test/clang-tidy/readability-non-const-parameter.cpp:245
@@ +244,3 @@
+  C c(p);
+}
+

I have added tests and fixed FPs in latest diff.


Comment at: test/clang-tidy/readability-non-const-parameter.cpp:252
@@ +251,3 @@
+
+class Warn {
+public:

I have added a test in latest diff. but there is a false positive. I believe I 
heard about whole program analysis in clang-tidy sometime.. is that something 
worked on / implemented yet?



https://reviews.llvm.org/D15332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D15332: new clang-tidy checker readability-non-const-parameter

[Daniel Marjamäki via cfe-commits]

danielmarjamaki updated this revision to Diff 67504.
danielmarjamaki added a comment.

fixed more review comments


https://reviews.llvm.org/D15332

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/NonConstParameterCheck.cpp
  clang-tidy/readability/NonConstParameterCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-non-const-parameter.rst
  test/clang-tidy/readability-non-const-parameter.cpp

Index: test/clang-tidy/readability-non-const-parameter.cpp
===
--- test/clang-tidy/readability-non-const-parameter.cpp
+++ test/clang-tidy/readability-non-const-parameter.cpp
@@ -0,0 +1,283 @@
+// RUN: %check_clang_tidy %s readability-non-const-parameter %t
+
+// Currently the checker only warns about pointer arguments.
+//
+// It can be defined both that the data is const and that the pointer is const,
+// the checker only checks if the data can be const-specified.
+//
+// It does not warn about pointers to records or function pointers.
+
+// Some external function where first argument is nonconst and second is const.
+char *strcpy1(char *dest, const char *src);
+unsigned my_strcpy(char *buf, const char *s);
+unsigned my_strlen(const char *buf);
+
+// CHECK-MESSAGES: :[[@LINE+1]]:29: warning: pointer parameter 'last' can be pointer to const [readability-non-const-parameter]
+void warn1(int *first, int *last) {
+  // CHECK-FIXES: {{^}}void warn1(int *first, const int *last) {{{$}}
+  *first = 0;
+  if (first < last) {
+  } // <- last can be const
+}
+
+// TODO: warning should be written here
+void warn2(char *p) {
+  char buf[10];
+  strcpy1(buf, p);
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:19: warning: pointer parameter 'p' can be
+void assign1(int *p) {
+  // CHECK-FIXES: {{^}}void assign1(const int *p) {{{$}}
+  const int *q;
+  q = p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:19: warning: pointer parameter 'p' can be
+void assign2(int *p) {
+  // CHECK-FIXES: {{^}}void assign2(const int *p) {{{$}}
+  const int *q;
+  q = p + 1;
+}
+
+void assign3(int *p) {
+  *p = 0;
+}
+
+void assign4(int *p) {
+  *p += 2;
+}
+
+void assign5(char *p) {
+  p[0] = 0;
+}
+
+void assign6(int *p) {
+  int *q;
+  q = p++;
+}
+
+void assign7(char *p) {
+  char *a, *b;
+  a = b = p;
+}
+
+void assign8(char *a, char *b) {
+  char *x;
+  x = (a ? a : b);
+}
+
+void assign9(unsigned char *str, const unsigned int i) {
+  unsigned char *p;
+  for (p = str + i; *p;) {
+  }
+}
+
+void assign10(int *buf) {
+  int i, *p;
+  for (i = 0, p = buf; i < 10; i++, p++) {
+*p = 1;
+  }
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:17: warning: pointer parameter 'p' can be
+void init1(int *p) {
+  // CHECK-FIXES: {{^}}void init1(const int *p) {{{$}}
+  const int *q = p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:17: warning: pointer parameter 'p' can be
+void init2(int *p) {
+  // CHECK-FIXES: {{^}}void init2(const int *p) {{{$}}
+  const int *q = p + 1;
+}
+
+void init3(int *p) {
+  int *q = p;
+}
+
+void init4(float *p) {
+  int *q = (int *)p;
+}
+
+void init5(int *p) {
+  int *i = p ? p : 0;
+}
+
+void init6(int *p) {
+  int *a[] = {p, p, 0};
+}
+
+void init7(int *p, int x) {
+  for (int *q = p + x - 1; 0; q++)
+;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:18: warning: pointer parameter 'p' can be
+int return1(int *p) {
+  // CHECK-FIXES: {{^}}int return1(const int *p) {{{$}}
+  return *p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:25: warning: pointer parameter 'p' can be
+const int *return2(int *p) {
+  // CHECK-FIXES: {{^}}const int *return2(const int *p) {{{$}}
+  return p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:25: warning: pointer parameter 'p' can be
+const int *return3(int *p) {
+  // CHECK-FIXES: {{^}}const int *return3(const int *p) {{{$}}
+  return p + 1;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:27: warning: pointer parameter 'p' can be
+const char *return4(char *p) {
+  // CHECK-FIXES: {{^}}const char *return4(const char *p) {{{$}}
+  return p ? p : "";
+}
+
+char *return5(char *s) {
+  return s;
+}
+
+char *return6(char *s) {
+  return s + 1;
+}
+
+char *return7(char *a, char *b) {
+  return a ? a : b;
+}
+
+char return8(int *p) {
+  return ++(*p);
+}
+
+void dontwarn1(int *p) {
+  ++(*p);
+}
+
+void dontwarn2(int *p) {
+  (*p)++;
+}
+
+int dontwarn3(_Atomic(int) * p) {
+  return *p;
+}
+
+void callFunction1(char *p) {
+  strcpy1(p, "abc");
+}
+
+void callFunction2(char *p) {
+  strcpy1([0], "abc");
+}
+
+void callFunction3(char *p) {
+  strcpy1(p + 2, "abc");
+}
+
+char *callFunction4(char *p) {
+  return strcpy1(p, "abc");
+}
+
+unsigned callFunction5(char *buf) {
+  unsigned len = my_strlen(buf);
+  return len + my_strcpy(buf, "abc");
+}
+
+void f6(int **p);
+void callFunction6(int *p) { f6(); }
+
+typedef union { void *v; } t;
+void f7(t obj);
+void callFunction7(int *p) {
+  f7((t){p});
+}
+
+void f8(int );
+void callFunction8(int *p) {
+  f8(*p);
+}
+
+// Don't warn about nonconst function 

Re: [PATCH] D15332: new clang-tidy checker readability-non-const-parameter

[Daniel Marjamäki via cfe-commits]

danielmarjamaki marked 10 inline comments as done.


Comment at: test/clang-tidy/readability-non-const-parameter.cpp:117-135
@@ +116,21 @@
+// CHECK-MESSAGES: :[[@LINE+1]]:18: warning: pointer parameter 'p' can be
+int return1(int *p) {
+  // CHECK-FIXES: {{^}}int return1(const int *p) {{{$}}
+  return *p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:25: warning: pointer parameter 'p' can be
+const int *return2(int *p) {
+  // CHECK-FIXES: {{^}}const int *return2(const int *p) {{{$}}
+  return p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:25: warning: pointer parameter 'p' can be
+const int *return3(int *p) {
+  // CHECK-FIXES: {{^}}const int *return3(const int *p) {{{$}}
+  return p + 1;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:27: warning: pointer parameter 'p' can be
+const char *return4(char *p) {
+  // CHECK-FIXES: {{^}}const char *return4(const char *p) {{{$}}

I have changed the message now. pointer parameter 'p' can be pointer to const

The name is the same. Do you think it would be better with 
PointerParameterConstnessCheck() perhaps?


https://reviews.llvm.org/D15332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D15332: new clang-tidy checker readability-non-const-parameter

[Daniel Marjamäki via cfe-commits]

danielmarjamaki updated this revision to Diff 67500.
danielmarjamaki added a comment.

Fix patch so it can be applied in latest trunk. Tried to fix review comments.


https://reviews.llvm.org/D15332

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/NonConstParameterCheck.cpp
  clang-tidy/readability/NonConstParameterCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-non-const-parameter.rst
  test/clang-tidy/readability-non-const-parameter.cpp

Index: test/clang-tidy/readability-non-const-parameter.cpp
===
--- test/clang-tidy/readability-non-const-parameter.cpp
+++ test/clang-tidy/readability-non-const-parameter.cpp
@@ -0,0 +1,251 @@
+// RUN: %check_clang_tidy %s readability-non-const-parameter %t
+
+// Currently the checker only warns about pointer arguments.
+//
+// It can be defined both that the data is const and that the pointer is const,
+// the checker only checks if the data can be const-specified.
+//
+// It does not warn about pointers to records or function pointers.
+
+// Some external function where first argument is nonconst and second is const.
+char *strcpy1(char *dest, const char *src);
+unsigned my_strcpy(char *buf, const char *s);
+unsigned my_strlen(const char *buf);
+
+// CHECK-MESSAGES: :[[@LINE+1]]:29: warning: pointer parameter 'last' can be pointer to const [readability-non-const-parameter]
+void warn1(int *first, int *last) {
+  // CHECK-FIXES: {{^}}void warn1(int *first, const int *last) {{{$}}
+  *first = 0;
+  if (first < last) {
+  } // <- last can be const
+}
+
+// TODO: warning should be written here
+void warn2(char *p) {
+  char buf[10];
+  strcpy1(buf, p);
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:19: warning: pointer parameter 'p' can be
+void assign1(int *p) {
+  // CHECK-FIXES: {{^}}void assign1(const int *p) {{{$}}
+  const int *q;
+  q = p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:19: warning: pointer parameter 'p' can be
+void assign2(int *p) {
+  // CHECK-FIXES: {{^}}void assign2(const int *p) {{{$}}
+  const int *q;
+  q = p + 1;
+}
+
+void assign3(int *p) {
+  *p = 0;
+}
+
+void assign4(int *p) {
+  *p += 2;
+}
+
+void assign5(char *p) {
+  p[0] = 0;
+}
+
+void assign6(int *p) {
+  int *q;
+  q = p++;
+}
+
+void assign7(char *p) {
+  char *a, *b;
+  a = b = p;
+}
+
+void assign8(char *a, char *b) {
+  char *x;
+  x = (a ? a : b);
+}
+
+void assign9(unsigned char *str, const unsigned int i) {
+  unsigned char *p;
+  for (p = str + i; *p;) {
+  }
+}
+
+void assign10(int *buf) {
+  int i, *p;
+  for (i = 0, p = buf; i < 10; i++, p++) {
+*p = 1;
+  }
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:17: warning: pointer parameter 'p' can be
+void init1(int *p) {
+  // CHECK-FIXES: {{^}}void init1(const int *p) {{{$}}
+  const int *q = p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:17: warning: pointer parameter 'p' can be
+void init2(int *p) {
+  // CHECK-FIXES: {{^}}void init2(const int *p) {{{$}}
+  const int *q = p + 1;
+}
+
+void init3(int *p) {
+  int *q = p;
+}
+
+void init4(float *p) {
+  int *q = (int *)p;
+}
+
+void init5(int *p) {
+  int *i = p ? p : 0;
+}
+
+void init6(int *p) {
+  int *a[] = {p, p, 0};
+}
+
+void init7(int *p, int x) {
+  for (int *q = p + x - 1; 0; q++)
+;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:18: warning: pointer parameter 'p' can be
+int return1(int *p) {
+  // CHECK-FIXES: {{^}}int return1(const int *p) {{{$}}
+  return *p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:25: warning: pointer parameter 'p' can be
+const int *return2(int *p) {
+  // CHECK-FIXES: {{^}}const int *return2(const int *p) {{{$}}
+  return p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:25: warning: pointer parameter 'p' can be
+const int *return3(int *p) {
+  // CHECK-FIXES: {{^}}const int *return3(const int *p) {{{$}}
+  return p + 1;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:27: warning: pointer parameter 'p' can be
+const char *return4(char *p) {
+  // CHECK-FIXES: {{^}}const char *return4(const char *p) {{{$}}
+  return p ? p : "";
+}
+
+char *return5(char *s) {
+  return s;
+}
+
+char *return6(char *s) {
+  return s + 1;
+}
+
+char *return7(char *a, char *b) {
+  return a ? a : b;
+}
+
+char return8(int *p) {
+  return ++(*p);
+}
+
+void dontwarn1(int *p) {
+  ++(*p);
+}
+
+void dontwarn2(int *p) {
+  (*p)++;
+}
+
+int dontwarn3(_Atomic(int) * p) {
+  return *p;
+}
+
+void callFunction1(char *p) {
+  strcpy1(p, "abc");
+}
+
+void callFunction2(char *p) {
+  strcpy1([0], "abc");
+}
+
+void callFunction3(char *p) {
+  strcpy1(p + 2, "abc");
+}
+
+char *callFunction4(char *p) {
+  return strcpy1(p, "abc");
+}
+
+unsigned callFunction5(char *buf) {
+  unsigned len = my_strlen(buf);
+  return len + my_strcpy(buf, "abc");
+}
+
+void f6(int **p);
+void callFunction6(int *p) { f6(); }
+
+typedef union { void *v; } t;
+void f7(t obj);
+void callFunction7(int *p) {
+  f7((t){p});
+}
+
+void f8(int );
+void callFunction8(int *p) {
+  

Re: [PATCH] D13126: New static analyzer checker for loss of sign/precision

[Daniel Marjamäki via cfe-commits]

danielmarjamaki updated this revision to Diff 67470.
danielmarjamaki added a comment.

Make sure patch can be applied in latest trunk.

Ping.


https://reviews.llvm.org/D13126

Files:
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/CMakeLists.txt
  lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
  test/Analysis/conversion.c

Index: test/Analysis/conversion.c
===
--- test/Analysis/conversion.c
+++ test/Analysis/conversion.c
@@ -0,0 +1,125 @@
+// RUN: %clang_cc1 -Wno-conversion -analyze -analyzer-checker=core,alpha.core.Conversion -verify %s
+
+unsigned char U8;
+signed char S8;
+
+void assign(unsigned U, signed S) {
+  if (S < -10)
+U8 = S; // expected-warning {{Loss of sign in implicit conversion}}
+  if (U > 300)
+S8 = U; // expected-warning {{Loss of precision in implicit conversion}}
+  if (S > 10)
+U8 = S;
+  if (U < 200)
+S8 = U;
+}
+
+void init1() {
+  long long A = 1LL << 60;
+  short X = A; // expected-warning {{Loss of precision in implicit conversion}}
+}
+
+void relational(unsigned U, signed S) {
+  if (S > 10) {
+if (U < S) {
+}
+  }
+  if (S < -10) {
+if (U < S) { // expected-warning {{Loss of sign in implicit conversion}}
+}
+  }
+}
+
+void multiplication(unsigned U, signed S) {
+  if (S > 5)
+S = U * S;
+  if (S < -10)
+S = U * S; // expected-warning {{Loss of sign}}
+}
+
+void division(unsigned U, signed S) {
+  if (S > 5)
+S = U / S;
+  if (S < -10)
+S = U / S; // expected-warning {{Loss of sign}}
+}
+
+void dontwarn1(unsigned U, signed S) {
+  U8 = S; // It might be known that S is always 0x00-0xff.
+  S8 = U; // It might be known that U is always 0x00-0xff.
+
+  U8 = -1;  // Explicit conversion.
+  S8 = ~0U; // Explicit conversion.
+  if (U > 300)
+U8 &= U; // No loss of precision since there is &=.
+}
+
+void dontwarn2(unsigned int U) {
+  if (U <= 4294967295) {
+  }
+  if (U <= (2147483647 * 2U + 1U)) {
+  }
+}
+
+void dontwarn3(int X) {
+  S8 = X ? 'a' : 'b';
+}
+
+// don't warn for macros
+#define DOSTUFF ({ unsigned X = 1000; U8 = X; })
+void dontwarn4() {
+  DOSTUFF;
+}
+
+// don't warn for calculations
+// seen some fp. For instance:  c2 = (c2 >= 'A' && c2 <= 'Z') ? c2 - 'A' + 'a' : c2;
+// there is a todo in the checker to handle calculations
+void dontwarn5() {
+  signed S = -32;
+  U8 = S + 10;
+}
+
+
+// false positives..
+
+int isascii(int c);
+void falsePositive1() {
+  char kb2[5];
+  int X = 1000;
+  if (isascii(X)) {
+// FIXME: should not warn here:
+kb2[0] = X; // expected-warning {{Loss of precision}}
+  }
+}
+
+
+typedef struct FILE {} FILE; int getc(FILE *stream);
+# define EOF (-1)
+char reply_string[8192];
+FILE *cin;
+extern int dostuff (void);
+int falsePositive2() {
+  int c, n;
+  int dig;
+  char *cp = reply_string;
+  int pflag = 0;
+  int code;
+
+  for (;;) {
+dig = n = code = 0;
+while ((c = getc(cin)) != '\n') {
+  if (dig < 4 && dostuff())
+code = code * 10 + (c - '0');
+  if (!pflag && code == 227)
+pflag = 1;
+  if (n == 0)
+n = c;
+  if (c == EOF)
+return(4);
+  if (cp < _string[sizeof(reply_string) - 1])
+// FIXME: should not warn here:
+*cp++ = c; // expected-warning {{Loss of precision}}
+}
+  }
+}
+
Index: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
+++ lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
@@ -0,0 +1,183 @@
+//=== ConversionChecker.cpp -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// Check that there is not loss of sign/precision in assignments, comparisons
+// and multiplications.
+//
+// ConversionChecker uses path sensitive analysis to determine possible values
+// of expressions. A warning is reported when:
+// * a negative value is implicitly converted to an unsigned value in an
+//   assignment, comparison or multiplication.
+// * assignment / initialization when source value is greater than the max
+//   value of target
+//
+// Many compilers and tools have similar checks that are based on semantic
+// analysis. Those checks are sound but has poor precision. ConversionChecker
+// is an alternative to those checks.
+//
+//===--===//
+#include "ClangSACheckers.h"
+#include "clang/AST/ParentMap.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+

Re: [PATCH] D21134: clang-tidy: new check readability-misplaced-array-index

[Daniel Marjamäki via cfe-commits]

danielmarjamaki marked an inline comment as done.
danielmarjamaki added a comment.

https://reviews.llvm.org/D21134



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D21134: clang-tidy: new check readability-misplaced-array-index

[Daniel Marjamäki via cfe-commits]

danielmarjamaki updated this revision to Diff 67338.
danielmarjamaki added a comment.

ran clang-format


https://reviews.llvm.org/D21134

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/MisplacedArrayIndexCheck.cpp
  clang-tidy/readability/MisplacedArrayIndexCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-misplaced-array-index.rst
  test/clang-tidy/readability-misplaced-array-index.cpp

Index: test/clang-tidy/readability-misplaced-array-index.cpp
===
--- test/clang-tidy/readability-misplaced-array-index.cpp
+++ test/clang-tidy/readability-misplaced-array-index.cpp
@@ -0,0 +1,35 @@
+// RUN: %check_clang_tidy %s readability-misplaced-array-index %t
+
+#define ABC  "abc"
+
+struct XY { int *x; int *y; };
+
+void dostuff(int);
+
+void unusualSyntax(int *x, struct XY *xy)
+{
+  10[x] = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: confusing array subscript expression, usually the index is inside the []
+  // CHECK-FIXES: x[10] = 0;
+
+  10[xy->x] = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: confusing array subscript expression, usually the index is inside the []
+  // CHECK-FIXES: xy->x[10] = 0;
+
+  dostuff(1["abc"]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: confusing array subscript expression, usually the index is inside the []
+  // CHECK-FIXES:  dostuff("abc"[1]);
+
+  dostuff(1[ABC]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: confusing array subscript expression, usually the index is inside the []
+  // No fixit, we don't want to replace with "abc"[1]
+
+  dostuff(0[0 + ABC]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: confusing array subscript expression, usually the index is inside the []
+  // No fixit. Probably the code should be ABC[0]
+}
+
+void normalSyntax(int *x)
+{
+  x[10] = 0;
+}
Index: docs/clang-tidy/checks/readability-misplaced-array-index.rst
===
--- docs/clang-tidy/checks/readability-misplaced-array-index.rst
+++ docs/clang-tidy/checks/readability-misplaced-array-index.rst
@@ -0,0 +1,29 @@
+.. title:: clang-tidy - readability-misplaced-array-index
+
+readability-misplaced-array-index
+=
+
+This check warns for unusual array index syntax.
+
+The following code has unusual array index syntax:
+
+.. code:: c++
+
+  void f(int *x, int y)
+  {
+y[x] = 0;
+  }
+
+becomes
+
+.. code:: c++
+
+  void f(int *x, int y)
+  {
+x[y] = 0;
+  }
+
+The check warns about such unusual syntax for readability reasons:
+ * There are programmers that are not familiar with this unusual syntax.
+ * It is possible that variables are mixed up.
+
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -126,6 +126,7 @@
readability-identifier-naming
readability-implicit-bool-cast
readability-inconsistent-declaration-parameter-name
+   readability-misplaced-array-index
readability-named-parameter
readability-redundant-control-flow
readability-redundant-smartptr-get
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -75,6 +75,10 @@
   This check warns about the performance overhead arising from concatenating
   strings using the ``operator+``, instead of ``operator+=``.
 
+- New `readability-misplaced-array-index
+  `_ check
+
+  Warns when there is array index before the [] instead of inside it.
 
 Improvements to include-fixer
 -
Index: clang-tidy/readability/ReadabilityTidyModule.cpp
===
--- clang-tidy/readability/ReadabilityTidyModule.cpp
+++ clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -19,6 +19,7 @@
 #include "IdentifierNamingCheck.h"
 #include "ImplicitBoolCastCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"
+#include "MisplacedArrayIndexCheck.h"
 #include "NamedParameterCheck.h"
 #include "RedundantControlFlowCheck.h"
 #include "RedundantSmartptrGetCheck.h"
@@ -53,6 +54,8 @@
 "readability-implicit-bool-cast");
 CheckFactories.registerCheck(
 "readability-inconsistent-declaration-parameter-name");
+CheckFactories.registerCheck(
+"readability-misplaced-array-index");
 CheckFactories.registerCheck(
 "readability-static-definition-in-anonymous-namespace");
 CheckFactories.registerCheck(
Index: clang-tidy/readability/MisplacedArrayIndexCheck.h
===
--- clang-tidy/readability/MisplacedArrayIndexCheck.h
+++ 

Re: [PATCH] D21134: clang-tidy: new check readability-misplaced-array-index

[Daniel Marjamäki via cfe-commits]

danielmarjamaki updated this revision to Diff 67337.
danielmarjamaki added a comment.

More generic diagnostic. Diagnose all integerType[pointerType] expressions.


https://reviews.llvm.org/D21134

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/MisplacedArrayIndexCheck.cpp
  clang-tidy/readability/MisplacedArrayIndexCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-misplaced-array-index.rst
  test/clang-tidy/readability-misplaced-array-index.cpp

Index: test/clang-tidy/readability-misplaced-array-index.cpp
===
--- test/clang-tidy/readability-misplaced-array-index.cpp
+++ test/clang-tidy/readability-misplaced-array-index.cpp
@@ -0,0 +1,35 @@
+// RUN: %check_clang_tidy %s readability-misplaced-array-index %t
+
+#define ABC  "abc"
+
+struct XY { int *x; int *y; };
+
+void dostuff(int);
+
+void unusualSyntax(int *x, struct XY *xy)
+{
+  10[x] = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: confusing array subscript expression, usually the index is inside the []
+  // CHECK-FIXES: x[10] = 0;
+
+  10[xy->x] = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: confusing array subscript expression, usually the index is inside the []
+  // CHECK-FIXES: xy->x[10] = 0;
+
+  dostuff(1["abc"]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: confusing array subscript expression, usually the index is inside the []
+  // CHECK-FIXES:  dostuff("abc"[1]);
+
+  dostuff(1[ABC]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: confusing array subscript expression, usually the index is inside the []
+  // No fixit, we don't want to replace with "abc"[1]
+
+  dostuff(0[0 + ABC]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: confusing array subscript expression, usually the index is inside the []
+  // No fixit. Probably the code should be ABC[0]
+}
+
+void normalSyntax(int *x)
+{
+  x[10] = 0;
+}
Index: docs/clang-tidy/checks/readability-misplaced-array-index.rst
===
--- docs/clang-tidy/checks/readability-misplaced-array-index.rst
+++ docs/clang-tidy/checks/readability-misplaced-array-index.rst
@@ -0,0 +1,29 @@
+.. title:: clang-tidy - readability-misplaced-array-index
+
+readability-misplaced-array-index
+=
+
+This check warns for unusual array index syntax.
+
+The following code has unusual array index syntax:
+
+.. code:: c++
+
+  void f(int *x, int y)
+  {
+y[x] = 0;
+  }
+
+becomes
+
+.. code:: c++
+
+  void f(int *x, int y)
+  {
+x[y] = 0;
+  }
+
+The check warns about such unusual syntax for readability reasons:
+ * There are programmers that are not familiar with this unusual syntax.
+ * It is possible that variables are mixed up.
+
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -126,6 +126,7 @@
readability-identifier-naming
readability-implicit-bool-cast
readability-inconsistent-declaration-parameter-name
+   readability-misplaced-array-index
readability-named-parameter
readability-redundant-control-flow
readability-redundant-smartptr-get
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -75,6 +75,10 @@
   This check warns about the performance overhead arising from concatenating
   strings using the ``operator+``, instead of ``operator+=``.
 
+- New `readability-misplaced-array-index
+  `_ check
+
+  Warns when there is array index before the [] instead of inside it.
 
 Improvements to include-fixer
 -
Index: clang-tidy/readability/ReadabilityTidyModule.cpp
===
--- clang-tidy/readability/ReadabilityTidyModule.cpp
+++ clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -19,6 +19,7 @@
 #include "IdentifierNamingCheck.h"
 #include "ImplicitBoolCastCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"
+#include "MisplacedArrayIndexCheck.h"
 #include "NamedParameterCheck.h"
 #include "RedundantControlFlowCheck.h"
 #include "RedundantSmartptrGetCheck.h"
@@ -53,6 +54,8 @@
 "readability-implicit-bool-cast");
 CheckFactories.registerCheck(
 "readability-inconsistent-declaration-parameter-name");
+CheckFactories.registerCheck(
+"readability-misplaced-array-index");
 CheckFactories.registerCheck(
 "readability-static-definition-in-anonymous-namespace");
 CheckFactories.registerCheck(
Index: clang-tidy/readability/MisplacedArrayIndexCheck.h
===
--- 

Re: [PATCH] D21134: clang-tidy: new check readability-misplaced-array-index

[Daniel Marjamäki via cfe-commits]

danielmarjamaki marked 2 inline comments as done.


Comment at: test/clang-tidy/readability-misplaced-array-index.cpp:31
@@ +30,3 @@
+  x[10] = 0;
+  dostuff(0[0 + ABC]);
+}

aaron.ballman wrote:
> Why is this considered to be "normal syntax" and not worth getting a 
> diagnostic?
hmm.. I agree that it would be good to have a diagnostic for this code also.

I currently only diagnose when subscript is stringLiteral, declRefExpr or 
memberExpr. These are the cases when the code can be easily fixed by just 
replacing the expressions.

I will look into writing a diagnostic for this also.



https://reviews.llvm.org/D21134



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D21134: clang-tidy: new check readability-misplaced-array-index

[Daniel Marjamäki via cfe-commits]

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

as far as I see the only unsolved review comment now is about macros. if it can 
be handled better.



Comment at: clang-tidy/readability/MisplacedArrayIndexCheck.cpp:28-29
@@ +27,4 @@
+  anyOf(stringLiteral(), declRefExpr(hasType(pointerType())),
+memberExpr(hasType(pointerType()))
+  .bind("expr"),
+  this);

Yes thanks!


Comment at: clang-tidy/readability/MisplacedArrayIndexCheck.cpp:53
@@ +52,3 @@
+  if (hasMacroID(ArraySubscriptE) ||
+  
!Result.SourceManager->isWrittenInSameFile(ArraySubscriptE->getLocStart(),
+ ArraySubscriptE->getLocEnd()))

you can do lots of weird things with macros. so I wanted to just diagnose and 
then have no fixits that would be wrong etc. I removed the comment since I 
think the code is pretty clear.


Comment at: clang-tidy/readability/MisplacedArrayIndexCheck.cpp:55
@@ +54,3 @@
+ ArraySubscriptE->getLocEnd()))
+return;
+

> What exactly is the recursive hasMacroID function trying to prevent? Do you 
> have a test that breaks if you just check that the start location of the 
> expression is not a macro?

I am worried about macros anywhere. I did not want to apply fixes for this 
code:  0[ABC]  but maybe using the Lexer would make that safe.

> In most cases, it's enough to check that the whole range is on the same macro 
> expansion level using Lexer::makeFileCharRange in order to prevent most 
> problems with macros, while allowing fixes in safe cases, e.g. replacements 
> in parameters of EXPECT_* macros from gtest.

I was very unsuccessful with that. I must do something wrong...
```
  CharSourceRange LRange = Lexer::makeFileCharRange(
  CharSourceRange::getCharRange(
  ArraySubscriptE->getLHS()->getSourceRange()),
  Result.Context->getSourceManager(), Result.Context->getLangOpts());

  CharSourceRange RRange = Lexer::makeFileCharRange(
  CharSourceRange::getCharRange(
  ArraySubscriptE->getLHS()->getSourceRange()),
  Result.Context->getSourceManager(), Result.Context->getLangOpts());

  StringRef LText = Lexer::getSourceText(LRange, *Result.SourceManager,
Result.Context->getLangOpts());

  StringRef RText = Lexer::getSourceText(RRange, *Result.SourceManager,
Result.Context->getLangOpts());

  Diag << 
FixItHint::CreateReplacement(ArraySubscriptE->getLHS()->getSourceRange(),
   RText);
  Diag << 
FixItHint::CreateReplacement(ArraySubscriptE->getRHS()->getSourceRange(),
LText);
```
No fixits worked with that code, with or without macros.

Do you see what I did wrong?


https://reviews.llvm.org/D21134



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D21134: clang-tidy: new check readability-misplaced-array-index

[Daniel Marjamäki via cfe-commits]

danielmarjamaki updated this revision to Diff 67322.
danielmarjamaki added a comment.

take care of review comments


https://reviews.llvm.org/D21134

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/MisplacedArrayIndexCheck.cpp
  clang-tidy/readability/MisplacedArrayIndexCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-misplaced-array-index.rst
  test/clang-tidy/readability-misplaced-array-index.cpp

Index: test/clang-tidy/readability-misplaced-array-index.cpp
===
--- test/clang-tidy/readability-misplaced-array-index.cpp
+++ test/clang-tidy/readability-misplaced-array-index.cpp
@@ -0,0 +1,32 @@
+// RUN: %check_clang_tidy %s readability-misplaced-array-index %t
+
+#define ABC  "abc"
+
+struct XY { int *x; int *y; };
+
+void dostuff(int);
+
+void unusualSyntax(int *x, struct XY *xy)
+{
+  10[x] = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: confusing array subscript expression, usually the index is inside the []
+  // CHECK-FIXES: x[10] = 0;
+
+  10[xy->x] = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: confusing array subscript expression, usually the index is inside the []
+  // CHECK-FIXES: xy->x[10] = 0;
+
+  dostuff(1["abc"]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: confusing array subscript expression, usually the index is inside the []
+  // CHECK-FIXES:  dostuff("abc"[1]);
+
+  dostuff(1[ABC]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: confusing array subscript expression, usually the index is inside the []
+  // No fixit, we don't want to replace with "abc"[1]
+}
+
+void normalSyntax(int *x)
+{
+  x[10] = 0;
+  dostuff(0[0 + ABC]);
+}
Index: docs/clang-tidy/checks/readability-misplaced-array-index.rst
===
--- docs/clang-tidy/checks/readability-misplaced-array-index.rst
+++ docs/clang-tidy/checks/readability-misplaced-array-index.rst
@@ -0,0 +1,29 @@
+.. title:: clang-tidy - readability-misplaced-array-index
+
+readability-misplaced-array-index
+=
+
+This check warns for unusual array index syntax.
+
+The following code has unusual array index syntax:
+
+.. code:: c++
+
+  void f(int *x, int y)
+  {
+y[x] = 0;
+  }
+
+becomes
+
+.. code:: c++
+
+  void f(int *x, int y)
+  {
+x[y] = 0;
+  }
+
+The check warns about such unusual syntax for readability reasons:
+ * There are programmers that are not familiar with this unusual syntax.
+ * It is possible that variables are mixed up.
+
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -126,6 +126,7 @@
readability-identifier-naming
readability-implicit-bool-cast
readability-inconsistent-declaration-parameter-name
+   readability-misplaced-array-index
readability-named-parameter
readability-redundant-control-flow
readability-redundant-smartptr-get
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -75,6 +75,10 @@
   This check warns about the performance overhead arising from concatenating
   strings using the ``operator+``, instead of ``operator+=``.
 
+- New `readability-misplaced-array-index
+  `_ check
+
+  Warns when there is array index before the [] instead of inside it.
 
 Improvements to include-fixer
 -
Index: clang-tidy/readability/ReadabilityTidyModule.cpp
===
--- clang-tidy/readability/ReadabilityTidyModule.cpp
+++ clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -19,6 +19,7 @@
 #include "IdentifierNamingCheck.h"
 #include "ImplicitBoolCastCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"
+#include "MisplacedArrayIndexCheck.h"
 #include "NamedParameterCheck.h"
 #include "RedundantControlFlowCheck.h"
 #include "RedundantSmartptrGetCheck.h"
@@ -53,6 +54,8 @@
 "readability-implicit-bool-cast");
 CheckFactories.registerCheck(
 "readability-inconsistent-declaration-parameter-name");
+CheckFactories.registerCheck(
+"readability-misplaced-array-index");
 CheckFactories.registerCheck(
 "readability-static-definition-in-anonymous-namespace");
 CheckFactories.registerCheck(
Index: clang-tidy/readability/MisplacedArrayIndexCheck.h
===
--- clang-tidy/readability/MisplacedArrayIndexCheck.h
+++ clang-tidy/readability/MisplacedArrayIndexCheck.h
@@ -0,0 +1,36 @@
+//===--- MisplacedArrayIndexCheck.h - clang-tidy-*- C++ -*-===//
+//
+// The LLVM Compiler 

Re: [PATCH] D21134: clang-tidy: new check readability-misplaced-array-index

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a comment.

In https://reviews.llvm.org/D21134#508524, @jroelofs wrote:

> I think the replacement is wrong for something like:
>
>   int *arr; int offs1, offs2;
>   offs1[arr + offs2] = 42;
>
>
> which I think would give:
>
>   int *arr; int offs1, offs2;
>   arr + offs2[offs1] = 42;
>
>
> If the precedence of the "indexing" argument is less than that of the 
> subscript operator, then you need to insert parens:
>
>   int *arr; int offs1, offs2;
>   (arr + offs2)[offs1] = 42;
>


I don't think so. The matcher says:

  hasRHS(ignoringParenImpCasts(
anyOf(stringLiteral(), declRefExpr(hasType(pointsTo(qualType(,
  memberExpr(hasType(pointsTo(qualType()))

I think it's hard to know what the replaced code should be. In some cases it 
might be better with:

  arr[offs1 + offs2] = 42;


https://reviews.llvm.org/D21134



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D21134: clang-tidy: new check readability-misplaced-array-index

[Daniel Marjamäki via cfe-commits]

danielmarjamaki marked an inline comment as done.
danielmarjamaki added a comment.

In https://reviews.llvm.org/D21134#508511, @aaron.ballman wrote:

> Is there a reason we don't want this check to be a part of the clang 
> frontend, rather than as a clang-tidy check?


I discussed this with frontend and clang-tidy people in the llvm meeting last 
year and we came to the conclusion it was better in clang-tidy.

I don't remember the exact reasons.

But I think that this code is just "weird". It could be a bug but I write the 
warning mostly for readability reasons.



Comment at: clang-tidy/readability/MisplacedArrayIndexCheck.cpp:43
@@ +42,3 @@
+
+static StringRef getAsString(const MatchFinder::MatchResult ,
+ const Expr *E) {

danielmarjamaki wrote:
> alexfh wrote:
> > Looks like this repeats getText from clang/Tooling/FixIt.h.
> Yes indeed..
this is fixed. As you suggested I used createReplacement() in the fixit. It's 
much nicer!


https://reviews.llvm.org/D21134



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D21134: clang-tidy: new check readability-misplaced-array-index

[Daniel Marjamäki via cfe-commits]

danielmarjamaki removed rL LLVM as the repository for this revision.
danielmarjamaki updated this revision to Diff 67145.
danielmarjamaki added a comment.

Cleanup my previous commit. Ran clang-format.


https://reviews.llvm.org/D21134

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/MisplacedArrayIndexCheck.cpp
  clang-tidy/readability/MisplacedArrayIndexCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-misplaced-array-index.rst
  test/clang-tidy/readability-misplaced-array-index.cpp

Index: test/clang-tidy/readability-misplaced-array-index.cpp
===
--- test/clang-tidy/readability-misplaced-array-index.cpp
+++ test/clang-tidy/readability-misplaced-array-index.cpp
@@ -0,0 +1,32 @@
+// RUN: %check_clang_tidy %s readability-misplaced-array-index %t
+
+#define ABC  "abc"
+
+struct XY { int *x; int *y; };
+
+void dostuff(int);
+
+void unusualSyntax(int *x, struct XY *xy)
+{
+  10[x] = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unusual array index syntax, usually the index is inside the []
+  // CHECK-FIXES: x[10] = 0;
+
+  10[xy->x] = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unusual array index syntax, usually the index is inside the []
+  // CHECK-FIXES: xy->x[10] = 0;
+
+  dostuff(1["abc"]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: unusual array index syntax, usually the index is inside the []
+  // CHECK-FIXES:  dostuff("abc"[1]);
+
+  dostuff(1[ABC]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: unusual array index syntax, usually the index is inside the []
+  // No fixit, we don't want to replace with "abc"[1]
+}
+
+void normalSyntax(int *x)
+{
+  x[10] = 0;
+  dostuff(0[0 + ABC]);
+}
Index: docs/clang-tidy/checks/readability-misplaced-array-index.rst
===
--- docs/clang-tidy/checks/readability-misplaced-array-index.rst
+++ docs/clang-tidy/checks/readability-misplaced-array-index.rst
@@ -0,0 +1,29 @@
+.. title:: clang-tidy - readability-misplaced-array-index
+
+readability-misplaced-array-index
+=
+
+This check warns for unusual array index syntax.
+
+The following code has unusual array index syntax:
+
+.. code:: c++
+
+  void f(int *x, int y)
+  {
+y[x] = 0;
+  }
+
+becomes
+
+.. code:: c++
+
+  void f(int *x, int y)
+  {
+x[y] = 0;
+  }
+
+The check warns about such unusual syntax for readability reasons:
+ * There are programmers that are not familiar with this unusual syntax.
+ * It is possible that variables are mixed up.
+
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -126,6 +126,7 @@
readability-identifier-naming
readability-implicit-bool-cast
readability-inconsistent-declaration-parameter-name
+   readability-misplaced-array-index
readability-named-parameter
readability-redundant-control-flow
readability-redundant-smartptr-get
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -75,6 +75,10 @@
   This check warns about the performance overhead arising from concatenating
   strings using the ``operator+``, instead of ``operator+=``.
 
+- New `readability-misplaced-array-index
+  `_ check
+
+  Warns when there is array index before the [] instead of inside it.
 
 Improvements to include-fixer
 -
Index: clang-tidy/readability/ReadabilityTidyModule.cpp
===
--- clang-tidy/readability/ReadabilityTidyModule.cpp
+++ clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -19,6 +19,7 @@
 #include "IdentifierNamingCheck.h"
 #include "ImplicitBoolCastCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"
+#include "MisplacedArrayIndexCheck.h"
 #include "NamedParameterCheck.h"
 #include "RedundantControlFlowCheck.h"
 #include "RedundantSmartptrGetCheck.h"
@@ -53,6 +54,8 @@
 "readability-implicit-bool-cast");
 CheckFactories.registerCheck(
 "readability-inconsistent-declaration-parameter-name");
+CheckFactories.registerCheck(
+"readability-misplaced-array-index");
 CheckFactories.registerCheck(
 "readability-static-definition-in-anonymous-namespace");
 CheckFactories.registerCheck(
Index: clang-tidy/readability/MisplacedArrayIndexCheck.h
===
--- clang-tidy/readability/MisplacedArrayIndexCheck.h
+++ clang-tidy/readability/MisplacedArrayIndexCheck.h
@@ -0,0 +1,36 @@
+//===--- MisplacedArrayIndexCheck.h - clang-tidy-*- C++ -*-===//
+//

Re: [PATCH] D21134: clang-tidy: new check readability-misplaced-array-index

[Daniel Marjamäki via cfe-commits]

danielmarjamaki set the repository for this revision to rL LLVM.
danielmarjamaki updated this revision to Diff 67144.
danielmarjamaki added a comment.

Fixed review comments.


Repository:
  rL LLVM

https://reviews.llvm.org/D21134

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/MisplacedArrayIndexCheck.cpp
  clang-tidy/readability/MisplacedArrayIndexCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-misplaced-array-index.rst
  test/clang-tidy/readability-misplaced-array-index.cpp

Index: test/clang-tidy/readability-misplaced-array-index.cpp
===
--- test/clang-tidy/readability-misplaced-array-index.cpp
+++ test/clang-tidy/readability-misplaced-array-index.cpp
@@ -0,0 +1,32 @@
+// RUN: %check_clang_tidy %s readability-misplaced-array-index %t
+
+#define ABC  "abc"
+
+struct XY { int *x; int *y; };
+
+void dostuff(int);
+
+void unusualSyntax(int *x, struct XY *xy)
+{
+  10[x] = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unusual array index syntax, usually the index is inside the []
+  // CHECK-FIXES: x[10] = 0;
+
+  10[xy->x] = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unusual array index syntax, usually the index is inside the []
+  // CHECK-FIXES: xy->x[10] = 0;
+
+  dostuff(1["abc"]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: unusual array index syntax, usually the index is inside the []
+  // CHECK-FIXES:  dostuff("abc"[1]);
+
+  dostuff(1[ABC]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: unusual array index syntax, usually the index is inside the []
+  // No fixit, we don't want to replace with "abc"[1]
+}
+
+void normalSyntax(int *x)
+{
+  x[10] = 0;
+  dostuff(0[0 + ABC]);
+}
Index: docs/clang-tidy/checks/readability-misplaced-array-index.rst
===
--- docs/clang-tidy/checks/readability-misplaced-array-index.rst
+++ docs/clang-tidy/checks/readability-misplaced-array-index.rst
@@ -0,0 +1,29 @@
+.. title:: clang-tidy - readability-misplaced-array-index
+
+readability-misplaced-array-index
+=
+
+This check warns for unusual array index syntax.
+
+The following code has unusual array index syntax:
+
+.. code:: c++
+
+  void f(int *x, int y)
+  {
+y[x] = 0;
+  }
+
+becomes
+
+.. code:: c++
+
+  void f(int *x, int y)
+  {
+x[y] = 0;
+  }
+
+The check warns about such unusual syntax for readability reasons:
+ * There are programmers that are not familiar with this unusual syntax.
+ * It is possible that variables are mixed up.
+
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -126,6 +126,7 @@
readability-identifier-naming
readability-implicit-bool-cast
readability-inconsistent-declaration-parameter-name
+   readability-misplaced-array-index
readability-named-parameter
readability-redundant-control-flow
readability-redundant-smartptr-get
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -75,6 +75,10 @@
   This check warns about the performance overhead arising from concatenating
   strings using the ``operator+``, instead of ``operator+=``.
 
+- New `readability-misplaced-array-index
+  `_ check
+
+  Warns when there is array index before the [] instead of inside it.
 
 Improvements to include-fixer
 -
Index: clang-tidy/readability/ReadabilityTidyModule.cpp
===
--- clang-tidy/readability/ReadabilityTidyModule.cpp
+++ clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -19,6 +19,7 @@
 #include "IdentifierNamingCheck.h"
 #include "ImplicitBoolCastCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"
+#include "MisplacedArrayIndexCheck.h"
 #include "NamedParameterCheck.h"
 #include "RedundantControlFlowCheck.h"
 #include "RedundantSmartptrGetCheck.h"
@@ -53,6 +54,8 @@
 "readability-implicit-bool-cast");
 CheckFactories.registerCheck(
 "readability-inconsistent-declaration-parameter-name");
+CheckFactories.registerCheck(
+"readability-misplaced-array-index");
 CheckFactories.registerCheck(
 "readability-static-definition-in-anonymous-namespace");
 CheckFactories.registerCheck(
Index: clang-tidy/readability/MisplacedArrayIndexCheck.h
===
--- clang-tidy/readability/MisplacedArrayIndexCheck.h
+++ clang-tidy/readability/MisplacedArrayIndexCheck.h
@@ -0,0 +1,36 @@
+//===--- MisplacedArrayIndexCheck.h - clang-tidy-*- C++ -*-===//
+//
+//  

Re: [PATCH] D21134: clang-tidy: new check readability-misplaced-array-index

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added inline comments.


Comment at: clang-tidy/readability/MisplacedArrayIndexCheck.cpp:43
@@ +42,3 @@
+
+static StringRef getAsString(const MatchFinder::MatchResult ,
+ const Expr *E) {

alexfh wrote:
> Looks like this repeats getText from clang/Tooling/FixIt.h.
Yes indeed..


https://reviews.llvm.org/D21134



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D21134: clang-tidy: new check readability-misplaced-array-index

[Daniel Marjamäki via cfe-commits]

danielmarjamaki updated this revision to Diff 60804.
danielmarjamaki added a comment.

fixed typo in releasenotes


http://reviews.llvm.org/D21134

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/MisplacedArrayIndexCheck.cpp
  clang-tidy/readability/MisplacedArrayIndexCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-misplaced-array-index.rst
  test/clang-tidy/readability-misplaced-array-index.cpp

Index: test/clang-tidy/readability-misplaced-array-index.cpp
===
--- test/clang-tidy/readability-misplaced-array-index.cpp
+++ test/clang-tidy/readability-misplaced-array-index.cpp
@@ -0,0 +1,32 @@
+// RUN: %check_clang_tidy %s readability-misplaced-array-index %t
+
+#define ABC  "abc"
+
+struct XY { int *x; int *y; };
+
+void dostuff(int);
+
+void unusualSyntax(int *x, struct XY *xy)
+{
+  10[x] = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unusual array index syntax, usually the index is inside the []
+  // CHECK-FIXES: x[10] = 0;
+
+  10[xy->x] = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unusual array index syntax, usually the index is inside the []
+  // CHECK-FIXES: xy->x[10] = 0;
+
+  dostuff(1["abc"]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: unusual array index syntax, usually the index is inside the []
+  // CHECK-FIXES: dostuff("abc"[1]);
+
+  dostuff(1[ABC]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: unusual array index syntax, usually the index is inside the []
+  // No fixit, we don't want to replace with "abc"[1]
+}
+
+void normalSyntax(int *x)
+{
+  x[10] = 0;
+  dostuff(0[0 + ABC]);
+}
Index: docs/clang-tidy/checks/readability-misplaced-array-index.rst
===
--- docs/clang-tidy/checks/readability-misplaced-array-index.rst
+++ docs/clang-tidy/checks/readability-misplaced-array-index.rst
@@ -0,0 +1,29 @@
+.. title:: clang-tidy - readability-misplaced-array-index
+
+readability-misplaced-array-index
+=
+
+This check warns for unusual array index syntax.
+
+The following code has unusual array index syntax:
+
+.. code:: c++
+
+  void f(int *x, int y)
+  {
+y[x] = 0;
+  }
+
+becomes
+
+.. code:: c++
+
+  void f(int *x, int y)
+  {
+x[y] = 0;
+  }
+
+The check warns about such unusual syntax for readability reasons:
+ * There are programmers that are not familiar with this unusual syntax.
+ * It is possible that variables are mixed up.
+
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -120,6 +120,7 @@
readability-identifier-naming
readability-implicit-bool-cast
readability-inconsistent-declaration-parameter-name
+   readability-misplaced-array-index
readability-named-parameter
readability-redundant-control-flow
readability-redundant-smartptr-get
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -249,6 +249,11 @@
   Warns about defaulted constructors and assignment operators that are actually
   deleted.
 
+- New `readability-misplaced-array-index
+  `_ check
+
+  Warns when there is array index before the [] instead of inside it.
+
 - New `readability-redundant-control-flow
   `_ check
 
Index: clang-tidy/readability/ReadabilityTidyModule.cpp
===
--- clang-tidy/readability/ReadabilityTidyModule.cpp
+++ clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -19,6 +19,7 @@
 #include "IdentifierNamingCheck.h"
 #include "ImplicitBoolCastCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"
+#include "MisplacedArrayIndexCheck.h"
 #include "NamedParameterCheck.h"
 #include "RedundantControlFlowCheck.h"
 #include "RedundantSmartptrGetCheck.h"
@@ -53,6 +54,8 @@
 "readability-implicit-bool-cast");
 CheckFactories.registerCheck(
 "readability-inconsistent-declaration-parameter-name");
+CheckFactories.registerCheck(
+"readability-misplaced-array-index");
 CheckFactories.registerCheck(
 "readability-static-definition-in-anonymous-namespace");
 CheckFactories.registerCheck(
Index: clang-tidy/readability/MisplacedArrayIndexCheck.h
===
--- clang-tidy/readability/MisplacedArrayIndexCheck.h
+++ clang-tidy/readability/MisplacedArrayIndexCheck.h
@@ -0,0 +1,36 @@
+//===--- MisplacedArrayIndexCheck.h - clang-tidy-*- C++ -*-===//
+//
+// The LLVM Compiler 

Re: [PATCH] D21134: clang-tidy: new check readability-misplaced-array-index

[Daniel Marjamäki via cfe-commits]

danielmarjamaki removed rL LLVM as the repository for this revision.
danielmarjamaki updated this revision to Diff 60803.
danielmarjamaki added a comment.

updated releasedocs
deeper lookup if macro is used
only warn when the replacement can be done safely. the expressions "x[y+z]" and 
y+z[x]" are not the same.


http://reviews.llvm.org/D21134

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/MisplacedArrayIndexCheck.cpp
  clang-tidy/readability/MisplacedArrayIndexCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-misplaced-array-index.rst
  test/clang-tidy/readability-misplaced-array-index.cpp

Index: test/clang-tidy/readability-misplaced-array-index.cpp
===
--- test/clang-tidy/readability-misplaced-array-index.cpp
+++ test/clang-tidy/readability-misplaced-array-index.cpp
@@ -0,0 +1,32 @@
+// RUN: %check_clang_tidy %s readability-misplaced-array-index %t
+
+#define ABC  "abc"
+
+struct XY { int *x; int *y; };
+
+void dostuff(int);
+
+void unusualSyntax(int *x, struct XY *xy)
+{
+  10[x] = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unusual array index syntax, usually the index is inside the []
+  // CHECK-FIXES: x[10] = 0;
+
+  10[xy->x] = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unusual array index syntax, usually the index is inside the []
+  // CHECK-FIXES: xy->x[10] = 0;
+
+  dostuff(1["abc"]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: unusual array index syntax, usually the index is inside the []
+  // CHECK-FIXES: dostuff("abc"[1]);
+
+  dostuff(1[ABC]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: unusual array index syntax, usually the index is inside the []
+  // No fixit, we don't want to replace with "abc"[1]
+}
+
+void normalSyntax(int *x)
+{
+  x[10] = 0;
+  dostuff(0[0 + ABC]);
+}
Index: docs/clang-tidy/checks/readability-misplaced-array-index.rst
===
--- docs/clang-tidy/checks/readability-misplaced-array-index.rst
+++ docs/clang-tidy/checks/readability-misplaced-array-index.rst
@@ -0,0 +1,29 @@
+.. title:: clang-tidy - readability-misplaced-array-index
+
+readability-misplaced-array-index
+=
+
+This check warns for unusual array index syntax.
+
+The following code has unusual array index syntax:
+
+.. code:: c++
+
+  void f(int *x, int y)
+  {
+y[x] = 0;
+  }
+
+becomes
+
+.. code:: c++
+
+  void f(int *x, int y)
+  {
+x[y] = 0;
+  }
+
+The check warns about such unusual syntax for readability reasons:
+ * There are programmers that are not familiar with this unusual syntax.
+ * It is possible that variables are mixed up.
+
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -120,6 +120,7 @@
readability-identifier-naming
readability-implicit-bool-cast
readability-inconsistent-declaration-parameter-name
+   readability-misplaced-array-index
readability-named-parameter
readability-redundant-control-flow
readability-redundant-smartptr-get
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -249,6 +249,11 @@
   Warns about defaulted constructors and assignment operators that are actually
   deleted.
 
+- New `misplaced-array-index
+  `_ check
+
+  Warns when there is array index before the [] instead of inside it.
+
 - New `readability-redundant-control-flow
   `_ check
 
Index: clang-tidy/readability/ReadabilityTidyModule.cpp
===
--- clang-tidy/readability/ReadabilityTidyModule.cpp
+++ clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -19,6 +19,7 @@
 #include "IdentifierNamingCheck.h"
 #include "ImplicitBoolCastCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"
+#include "MisplacedArrayIndexCheck.h"
 #include "NamedParameterCheck.h"
 #include "RedundantControlFlowCheck.h"
 #include "RedundantSmartptrGetCheck.h"
@@ -53,6 +54,8 @@
 "readability-implicit-bool-cast");
 CheckFactories.registerCheck(
 "readability-inconsistent-declaration-parameter-name");
+CheckFactories.registerCheck(
+"readability-misplaced-array-index");
 CheckFactories.registerCheck(
 "readability-static-definition-in-anonymous-namespace");
 CheckFactories.registerCheck(
Index: clang-tidy/readability/MisplacedArrayIndexCheck.h
===
--- clang-tidy/readability/MisplacedArrayIndexCheck.h
+++ 

[PATCH] D21134: clang-tidy: new check readability-misplaced-array-index

[Daniel Marjamäki via cfe-commits]

danielmarjamaki created this revision.
danielmarjamaki added a reviewer: alexfh.
danielmarjamaki added a subscriber: cfe-commits.
danielmarjamaki set the repository for this revision to rL LLVM.

this is a new check for clang-tidy that diagnoses when it see unusual array 
index syntax.

there is nothing wrong about such syntax so it's not a misc check. It's just a 
readability check.


Repository:
  rL LLVM

http://reviews.llvm.org/D21134

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/MisplacedArrayIndexCheck.cpp
  clang-tidy/readability/MisplacedArrayIndexCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-misplaced-array-index.rst
  test/clang-tidy/readability-misplaced-array-index.cpp

Index: test/clang-tidy/readability-misplaced-array-index.cpp
===
--- test/clang-tidy/readability-misplaced-array-index.cpp
+++ test/clang-tidy/readability-misplaced-array-index.cpp
@@ -0,0 +1,19 @@
+// RUN: %check_clang_tidy %s readability-misplaced-array-index %t
+
+#define ABC  "abc"
+
+int unusualSyntax(int *x)
+{
+  10[x] = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unusual array index syntax, usually the index is inside the []
+  // CHECK-FIXES: x[10] = 0;
+
+  return 1[ABC];
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: unusual array index syntax, usually the index is inside the []
+  // No fixit, we don't want to replace with "abc"[1]
+}
+
+int normalSyntax(int *x)
+{
+  return x[10];
+}
Index: docs/clang-tidy/checks/readability-misplaced-array-index.rst
===
--- docs/clang-tidy/checks/readability-misplaced-array-index.rst
+++ docs/clang-tidy/checks/readability-misplaced-array-index.rst
@@ -0,0 +1,29 @@
+.. title:: clang-tidy - readability-misplaced-array-index
+
+readability-misplaced-array-index
+=
+
+This check warns for unusual array index syntax.
+
+The following code has unusual array index syntax:
+
+.. code:: c++
+
+  void f(int *x, int y)
+  {
+y[x] = 0;
+  }
+
+becomes
+
+.. code:: c++
+
+  void f(int *x, int y)
+  {
+x[y] = 0;
+  }
+
+The check warns about such unusual syntax for readability reasons:
+ * There are programmers that are not familiar with this unusual syntax.
+ * It is possible that variables are mixed up.
+
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -120,6 +120,7 @@
readability-identifier-naming
readability-implicit-bool-cast
readability-inconsistent-declaration-parameter-name
+   readability-misplaced-array-index
readability-named-parameter
readability-redundant-control-flow
readability-redundant-smartptr-get
Index: clang-tidy/readability/ReadabilityTidyModule.cpp
===
--- clang-tidy/readability/ReadabilityTidyModule.cpp
+++ clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -19,6 +19,7 @@
 #include "IdentifierNamingCheck.h"
 #include "ImplicitBoolCastCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"
+#include "MisplacedArrayIndexCheck.h"
 #include "NamedParameterCheck.h"
 #include "RedundantControlFlowCheck.h"
 #include "RedundantSmartptrGetCheck.h"
@@ -53,6 +54,8 @@
 "readability-implicit-bool-cast");
 CheckFactories.registerCheck(
 "readability-inconsistent-declaration-parameter-name");
+CheckFactories.registerCheck(
+"readability-misplaced-array-index");
 CheckFactories.registerCheck(
 "readability-static-definition-in-anonymous-namespace");
 CheckFactories.registerCheck(
Index: clang-tidy/readability/MisplacedArrayIndexCheck.h
===
--- clang-tidy/readability/MisplacedArrayIndexCheck.h
+++ clang-tidy/readability/MisplacedArrayIndexCheck.h
@@ -0,0 +1,36 @@
+//===--- MisplacedArrayIndexCheck.h - clang-tidy-*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_MISPLACED_ARRAY_INDEX_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_MISPLACED_ARRAY_INDEX_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Warn about unusual array index syntax (index[array] instead of
+/// array[index]).
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-misplaced-array-index.html
+class MisplacedArrayIndexCheck : public ClangTidyCheck {
+public:
+  

Re: [PATCH] D20853: [clang-tidy] misc-macro-parentheses: Don't insert parentheses in variable declarations. Fixes bugzilla 26273

[Daniel Marjamäki via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL272128 (authored by danielmarjamaki).

Changed prior to commit:
  http://reviews.llvm.org/D20853?vs=59208=60013#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D20853

Files:
  clang-tools-extra/trunk/clang-tidy/misc/MacroParenthesesCheck.cpp
  clang-tools-extra/trunk/test/clang-tidy/misc-macro-parentheses.cpp

Index: clang-tools-extra/trunk/clang-tidy/misc/MacroParenthesesCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/misc/MacroParenthesesCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/misc/MacroParenthesesCheck.cpp
@@ -19,8 +19,7 @@
 namespace {
 class MacroParenthesesPPCallbacks : public PPCallbacks {
 public:
-  MacroParenthesesPPCallbacks(Preprocessor *PP,
-  MacroParenthesesCheck *Check)
+  MacroParenthesesPPCallbacks(Preprocessor *PP, MacroParenthesesCheck *Check)
   : PP(PP), Check(Check) {}
 
   void MacroDefined(const Token ,
@@ -67,8 +66,46 @@
tok::amp, tok::pipe, tok::caret);
 }
 
+/// Is given Token a keyword that is used in variable declarations?
+static bool isVarDeclKeyword(const Token ) {
+  return T.isOneOf(tok::kw_bool, tok::kw_char, tok::kw_short, tok::kw_int,
+   tok::kw_long, tok::kw_float, tok::kw_double, tok::kw_const,
+   tok::kw_enum, tok::kw_inline, tok::kw_static, tok::kw_struct,
+   tok::kw_signed, tok::kw_unsigned);
+}
+
+/// Is there a possible variable declaration at Tok?
+static bool possibleVarDecl(const MacroInfo *MI, const Token *Tok) {
+  if (Tok == MI->tokens_end())
+return false;
+
+  // If we see int/short/struct/etc., just assume this is a variable declaration.
+  if (isVarDeclKeyword(*Tok))
+return true;
+
+  // Variable declarations start with identifier or coloncolon.
+  if (!Tok->isOneOf(tok::identifier, tok::raw_identifier, tok::coloncolon))
+return false;
+
+  // Skip possible types, etc
+  while (
+  Tok != MI->tokens_end() &&
+  Tok->isOneOf(tok::identifier, tok::raw_identifier, tok::coloncolon,
+tok::star, tok::amp, tok::ampamp, tok::less, tok::greater))
+Tok++;
+
+  // Return true for possible variable declarations.
+  return Tok == MI->tokens_end() ||
+ Tok->isOneOf(tok::equal, tok::semi, tok::l_square, tok::l_paren) ||
+ isVarDeclKeyword(*Tok);
+}
+
 void MacroParenthesesPPCallbacks::replacementList(const Token ,
   const MacroInfo *MI) {
+  // Make sure macro replacement isn't a variable declaration.
+  if (possibleVarDecl(MI, MI->tokens_begin()))
+return;
+
   // Count how deep we are in parentheses/braces/squares.
   int Count = 0;
 
@@ -117,6 +154,9 @@
 
 void MacroParenthesesPPCallbacks::argument(const Token ,
const MacroInfo *MI) {
+  
+  // Skip variable declaration.
+  bool VarDecl = possibleVarDecl(MI, MI->tokens_begin());
 
   for (auto TI = MI->tokens_begin(), TE = MI->tokens_end(); TI != TE; ++TI) {
 // First token.
@@ -132,6 +172,13 @@
 
 const Token  = *TI;
 
+// There should not be extra parentheses in possible variable declaration.
+if (VarDecl) {
+  if (Tok.isOneOf(tok::equal, tok::semi, tok::l_square, tok::l_paren))
+VarDecl = false;
+  continue;
+}
+
 // Only interested in identifiers.
 if (!Tok.isOneOf(tok::identifier, tok::raw_identifier))
   continue;
Index: clang-tools-extra/trunk/test/clang-tidy/misc-macro-parentheses.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/misc-macro-parentheses.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/misc-macro-parentheses.cpp
@@ -8,6 +8,8 @@
 // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: macro argument should be enclosed in parentheses [misc-macro-parentheses]
 #define BAD4(x)   ((unsigned char)(x & 0xff))
 // CHECK-MESSAGES: :[[@LINE-1]]:44: warning: macro argument should be enclosed in parentheses [misc-macro-parentheses]
+#define BAD5(X)   A*B=(C*)X+2
+// CHECK-MESSAGES: :[[@LINE-1]]:35: warning: macro argument should be enclosed in parentheses [misc-macro-parentheses]
 
 #define GOOD1 1
 #define GOOD2 (1+2)
@@ -39,6 +41,8 @@
 #define GOOD28(x) namespace x {int b;}
 #define GOOD29(...)   std::cout << __VA_ARGS__;
 #define GOOD30(args...)   std::cout << args;
+#define GOOD31(X) A*X=2
+#define GOOD32(X) std::vector
 
 // These are allowed for now..
 #define MAYBE1*12.34
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20853: [clang-tidy] misc-macro-parentheses: Don't insert parentheses in variable declarations. Fixes bugzilla 26273

[Daniel Marjamäki via cfe-commits]

danielmarjamaki removed rL LLVM as the repository for this revision.
danielmarjamaki updated this revision to Diff 59208.
danielmarjamaki added a comment.

Updated diff. Now also handles bugzilla 27399.


http://reviews.llvm.org/D20853

Files:
  clang-tidy/misc/MacroParenthesesCheck.cpp
  test/clang-tidy/misc-macro-parentheses.cpp

Index: test/clang-tidy/misc-macro-parentheses.cpp
===
--- test/clang-tidy/misc-macro-parentheses.cpp
+++ test/clang-tidy/misc-macro-parentheses.cpp
@@ -8,6 +8,8 @@
 // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: macro argument should be enclosed in parentheses [misc-macro-parentheses]
 #define BAD4(x)   ((unsigned char)(x & 0xff))
 // CHECK-MESSAGES: :[[@LINE-1]]:44: warning: macro argument should be enclosed in parentheses [misc-macro-parentheses]
+#define BAD5(X)   A*B=(C*)X+2
+// CHECK-MESSAGES: :[[@LINE-1]]:35: warning: macro argument should be enclosed in parentheses [misc-macro-parentheses]
 
 #define GOOD1 1
 #define GOOD2 (1+2)
@@ -39,6 +41,8 @@
 #define GOOD28(x) namespace x {int b;}
 #define GOOD29(...)   std::cout << __VA_ARGS__;
 #define GOOD30(args...)   std::cout << args;
+#define GOOD31(X) A*X=2
+#define GOOD32(X) std::vector
 
 // These are allowed for now..
 #define MAYBE1*12.34
Index: clang-tidy/misc/MacroParenthesesCheck.cpp
===
--- clang-tidy/misc/MacroParenthesesCheck.cpp
+++ clang-tidy/misc/MacroParenthesesCheck.cpp
@@ -19,8 +19,7 @@
 namespace {
 class MacroParenthesesPPCallbacks : public PPCallbacks {
 public:
-  MacroParenthesesPPCallbacks(Preprocessor *PP,
-  MacroParenthesesCheck *Check)
+  MacroParenthesesPPCallbacks(Preprocessor *PP, MacroParenthesesCheck *Check)
   : PP(PP), Check(Check) {}
 
   void MacroDefined(const Token ,
@@ -67,8 +66,46 @@
tok::amp, tok::pipe, tok::caret);
 }
 
+/// Is given Token a keyword that is used in variable declarations?
+static bool isVarDeclKeyword(const Token ) {
+  return T.isOneOf(tok::kw_bool, tok::kw_char, tok::kw_short, tok::kw_int,
+   tok::kw_long, tok::kw_float, tok::kw_double, tok::kw_const,
+   tok::kw_enum, tok::kw_inline, tok::kw_static, tok::kw_struct,
+   tok::kw_signed, tok::kw_unsigned);
+}
+
+/// Is there a possible variable declaration at Tok?
+static bool possibleVarDecl(const MacroInfo *MI, const Token *Tok) {
+  if (Tok == MI->tokens_end())
+return false;
+
+  // If we see int/short/struct etc just assume this is a variable declaration
+  if (isVarDeclKeyword(*Tok))
+return true;
+
+  // Variable declarations start with identifier or coloncolon
+  if (!Tok->isOneOf(tok::identifier, tok::raw_identifier, tok::coloncolon))
+return false;
+
+  // Skip possible types, etc
+  while (
+  Tok != MI->tokens_end() &&
+  (Tok->isOneOf(tok::identifier, tok::raw_identifier, tok::coloncolon,
+tok::star, tok::amp, tok::ampamp, tok::less, tok::greater)))
+Tok++;
+
+  // Return true for possible variable declarations
+  return Tok == MI->tokens_end() ||
+ Tok->isOneOf(tok::equal, tok::semi, tok::l_square, tok::l_paren) ||
+ isVarDeclKeyword(*Tok);
+}
+
 void MacroParenthesesPPCallbacks::replacementList(const Token ,
   const MacroInfo *MI) {
+  // Make sure macro replacement isn't a variable declaration
+  if (possibleVarDecl(MI, MI->tokens_begin()))
+return;
+
   // Count how deep we are in parentheses/braces/squares.
   int Count = 0;
 
@@ -117,6 +154,9 @@
 
 void MacroParenthesesPPCallbacks::argument(const Token ,
const MacroInfo *MI) {
+  
+  // Skip variable declaration
+  bool VarDecl = possibleVarDecl(MI, MI->tokens_begin());
 
   for (auto TI = MI->tokens_begin(), TE = MI->tokens_end(); TI != TE; ++TI) {
 // First token.
@@ -132,6 +172,13 @@
 
 const Token  = *TI;
 
+// There should not be extra parentheses in possible variable declaration.
+if (VarDecl) {
+  if (Tok.isOneOf(tok::equal, tok::semi, tok::l_square, tok::l_paren))
+VarDecl = false;
+  continue;
+}
+
 // Only interested in identifiers.
 if (!Tok.isOneOf(tok::identifier, tok::raw_identifier))
   continue;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20853: [clang-tidy] misc-macro-parentheses: Don't insert parentheses in variable declarations. Fixes bugzilla 26273

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a comment.

> The "possibleVarDecl" could be much more clever. For instance, variable 
> declarations can at least contain :: < & also, I could handle those better 
> also but that would mean more false negatives.


I now saw bugzilla 27399 also.

I do need to handle templates also.

so I will send a new patch with updated possibleVarDecl().


Repository:
  rL LLVM

http://reviews.llvm.org/D20853



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D20853: [clang-tidy] misc-macro-parentheses: Don't insert parentheses in variable declarations. Fixes bugzilla 26273

[Daniel Marjamäki via cfe-commits]

danielmarjamaki created this revision.
danielmarjamaki added subscribers: cfe-commits, alexfh.
danielmarjamaki set the repository for this revision to rL LLVM.

This is a quick fix for bugzilla 26273. parentheses should not be inserted in 
variable declarations.

This patch will introduce false negatives when a macro looks like it could be 
variable declaration but is not.

For example:

#define ABC   A*B*C

To avoid false positives in such expression, I am guessing that non-keywords 
such as "A", "B" and "C" are types or qualifiers so "A*B*C" is some variable 
declaration. My primary focus was to avoid FP.

Maybe there is a method that I am not aware of, to see if there is a type "A" 
during preprocessing.. that would be great.

The "possibleVarDecl" could be much more clever. For instance, variable 
declarations can at least contain :: < & also, I could handle those better also 
but that would mean more false negatives.


Repository:
  rL LLVM

http://reviews.llvm.org/D20853

Files:
  clang-tidy/misc/MacroParenthesesCheck.cpp
  test/clang-tidy/misc-macro-parentheses.cpp

Index: test/clang-tidy/misc-macro-parentheses.cpp
===
--- test/clang-tidy/misc-macro-parentheses.cpp
+++ test/clang-tidy/misc-macro-parentheses.cpp
@@ -8,6 +8,8 @@
 // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: macro argument should be enclosed 
in parentheses [misc-macro-parentheses]
 #define BAD4(x)   ((unsigned char)(x & 0xff))
 // CHECK-MESSAGES: :[[@LINE-1]]:44: warning: macro argument should be enclosed 
in parentheses [misc-macro-parentheses]
+#define BAD5(X)   A*B=(C*)X+2
+// CHECK-MESSAGES: :[[@LINE-1]]:35: warning: macro argument should be enclosed 
in parentheses [misc-macro-parentheses]
 
 #define GOOD1 1
 #define GOOD2 (1+2)
@@ -39,6 +41,7 @@
 #define GOOD28(x) namespace x {int b;}
 #define GOOD29(...)   std::cout << __VA_ARGS__;
 #define GOOD30(args...)   std::cout << args;
+#define GOOD31(X) A*X=2
 
 // These are allowed for now..
 #define MAYBE1*12.34
Index: clang-tidy/misc/MacroParenthesesCheck.cpp
===
--- clang-tidy/misc/MacroParenthesesCheck.cpp
+++ clang-tidy/misc/MacroParenthesesCheck.cpp
@@ -19,8 +19,7 @@
 namespace {
 class MacroParenthesesPPCallbacks : public PPCallbacks {
 public:
-  MacroParenthesesPPCallbacks(Preprocessor *PP,
-  MacroParenthesesCheck *Check)
+  MacroParenthesesPPCallbacks(Preprocessor *PP, MacroParenthesesCheck *Check)
   : PP(PP), Check(Check) {}
 
   void MacroDefined(const Token ,
@@ -67,8 +66,31 @@
tok::amp, tok::pipe, tok::caret);
 }
 
+static bool possibleVarDecl(const MacroInfo *MI) {
+  if (MI->tokens_empty())
+return false;
+
+  const Token *Tok = MI->tokens_begin();
+
+  // Skip possible unknown types
+  while (Tok != MI->tokens_end() && !isKeyword(*Tok) &&
+ Tok->isOneOf(tok::identifier, tok::raw_identifier) &&
+ MI->getArgumentNum(Tok->getIdentifierInfo()) < 0) {
+Tok++;
+  }
+
+  // Return true for 'UNKNOWN_ID*X', 'UNKNOWN_ID X' etc.
+  return Tok != MI->tokens_begin() && Tok != MI->tokens_end() && 
!isKeyword(*Tok) &&
+ Tok->isOneOf(tok::star, tok::identifier, tok::raw_identifier);
+}
+
 void MacroParenthesesPPCallbacks::replacementList(const Token ,
   const MacroInfo *MI) {
+  // Make sure macro replacement isn't a variable declaration, that begins
+  // with unknown identifier(s).
+  if (possibleVarDecl(MI))
+return;
+
   // Count how deep we are in parentheses/braces/squares.
   int Count = 0;
 
@@ -117,6 +139,7 @@
 
 void MacroParenthesesPPCallbacks::argument(const Token ,
const MacroInfo *MI) {
+  bool VarDecl = possibleVarDecl(MI);
 
   for (auto TI = MI->tokens_begin(), TE = MI->tokens_end(); TI != TE; ++TI) {
 // First token.
@@ -132,6 +155,13 @@
 
 const Token  = *TI;
 
+// Don't warn in possible variable declaration.
+if (VarDecl) {
+  if (Tok.is(tok::equal))
+VarDecl = false;
+  continue;
+}
+
 // Only interested in identifiers.
 if (!Tok.isOneOf(tok::identifier, tok::raw_identifier))
   continue;


Index: test/clang-tidy/misc-macro-parentheses.cpp
===
--- test/clang-tidy/misc-macro-parentheses.cpp
+++ test/clang-tidy/misc-macro-parentheses.cpp
@@ -8,6 +8,8 @@
 // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: macro argument should be enclosed in parentheses [misc-macro-parentheses]
 #define BAD4(x)   ((unsigned char)(x & 0xff))
 // CHECK-MESSAGES: :[[@LINE-1]]:44: warning: macro argument should be enclosed in parentheses [misc-macro-parentheses]
+#define BAD5(X)   A*B=(C*)X+2
+// CHECK-MESSAGES: :[[@LINE-1]]:35: warning: macro argument should be enclosed in parentheses 

Re: [PATCH] D13126: New static analyzer checker for loss of sign/precision

[Daniel Marjamäki via cfe-commits]

danielmarjamaki updated this revision to Diff 52029.
danielmarjamaki marked 2 inline comments as done.
danielmarjamaki added a comment.

Minor fixes of review comments. Renamed functions to "isNegative" and 
"isGreaterEqual" and return bool. Updated comment. Added testcases for false 
positives I have seen.


http://reviews.llvm.org/D13126

Files:
  lib/StaticAnalyzer/Checkers/CMakeLists.txt
  lib/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
  test/Analysis/conversion.c

Index: test/Analysis/conversion.c
===
--- test/Analysis/conversion.c
+++ test/Analysis/conversion.c
@@ -0,0 +1,125 @@
+// RUN: %clang_cc1 -Wno-conversion -analyze -analyzer-checker=core,alpha.core.Conversion -verify %s
+
+unsigned char U8;
+signed char S8;
+
+void assign(unsigned U, signed S) {
+  if (S < -10)
+U8 = S; // expected-warning {{Loss of sign in implicit conversion}}
+  if (U > 300)
+S8 = U; // expected-warning {{Loss of precision in implicit conversion}}
+  if (S > 10)
+U8 = S;
+  if (U < 200)
+S8 = U;
+}
+
+void init1() {
+  long long A = 1LL << 60;
+  short X = A; // expected-warning {{Loss of precision in implicit conversion}}
+}
+
+void relational(unsigned U, signed S) {
+  if (S > 10) {
+if (U < S) {
+}
+  }
+  if (S < -10) {
+if (U < S) { // expected-warning {{Loss of sign in implicit conversion}}
+}
+  }
+}
+
+void multiplication(unsigned U, signed S) {
+  if (S > 5)
+S = U * S;
+  if (S < -10)
+S = U * S; // expected-warning {{Loss of sign}}
+}
+
+void division(unsigned U, signed S) {
+  if (S > 5)
+S = U / S;
+  if (S < -10)
+S = U / S; // expected-warning {{Loss of sign}}
+}
+
+void dontwarn1(unsigned U, signed S) {
+  U8 = S; // It might be known that S is always 0x00-0xff.
+  S8 = U; // It might be known that U is always 0x00-0xff.
+
+  U8 = -1;  // Explicit conversion.
+  S8 = ~0U; // Explicit conversion.
+  if (U > 300)
+U8 &= U; // No loss of precision since there is &=.
+}
+
+void dontwarn2(unsigned int U) {
+  if (U <= 4294967295) {
+  }
+  if (U <= (2147483647 * 2U + 1U)) {
+  }
+}
+
+void dontwarn3(int X) {
+  S8 = X ? 'a' : 'b';
+}
+
+// don't warn for macros
+#define DOSTUFF ({ unsigned X = 1000; U8 = X; })
+void dontwarn4() {
+  DOSTUFF;
+}
+
+// don't warn for calculations
+// seen some fp. For instance:  c2 = (c2 >= 'A' && c2 <= 'Z') ? c2 - 'A' + 'a' : c2;
+// there is a todo in the checker to handle calculations
+void dontwarn5() {
+  signed S = -32;
+  U8 = S + 10;
+}
+
+
+// false positives..
+
+int isascii(int c);
+void falsePositive1() {
+  char kb2[5];
+  int X = 1000;
+  if (isascii(X)) {
+// FIXME: should not warn here:
+kb2[0] = X; // expected-warning {{Loss of precision}}
+  }
+}
+
+
+typedef struct FILE {} FILE; int getc(FILE *stream);
+# define EOF (-1)
+char reply_string[8192];
+FILE *cin;
+extern int dostuff (void);
+int falsePositive2() {
+  int c, n;
+  int dig;
+  char *cp = reply_string;
+  int pflag = 0;
+  int code;
+
+  for (;;) {
+dig = n = code = 0;
+while ((c = getc(cin)) != '\n') {
+  if (dig < 4 && dostuff())
+code = code * 10 + (c - '0');
+  if (!pflag && code == 227)
+pflag = 1;
+  if (n == 0)
+n = c;
+  if (c == EOF)
+return(4);
+  if (cp < _string[sizeof(reply_string) - 1])
+// FIXME: should not warn here:
+*cp++ = c; // expected-warning {{Loss of precision}}
+}
+  }
+}
+
Index: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
+++ lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
@@ -0,0 +1,183 @@
+//=== ConversionChecker.cpp -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// Check that there is not loss of sign/precision in assignments, comparisons
+// and multiplications.
+//
+// ConversionChecker uses path sensitive analysis to determine possible values
+// of expressions. A warning is reported when:
+// * a negative value is implicitly converted to an unsigned value in an
+//   assignment, comparison or multiplication.
+// * assignment / initialization when source value is greater than the max
+//   value of target
+//
+// Many compilers and tools have similar checks that are based on semantic
+// analysis. Those checks are sound but has poor precision. ConversionChecker
+// is an alternative to those checks.
+//
+//===--===//
+#include "ClangSACheckers.h"
+#include "clang/AST/ParentMap.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include 

Re: [PATCH] D13126: New static analyzer checker for loss of sign/precision

[Daniel Marjamäki via cfe-commits]

danielmarjamaki marked 3 inline comments as done.


Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:12
@@ +11,3 @@
+//
+// ConversionChecker generates a subset of the warnings that are reported by
+// Wconversion. It is designed to be an alternative to Wconversion.

Thanks! I have tried to do that.


Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:30
@@ +29,3 @@
+
+namespace {
+class ConversionChecker : public Checker {

ok


Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:85
@@ +84,3 @@
+  if (!N)
+return;
+

I renamed and changed these functions. Hope we all like it better now. The name 
is now "greaterEqualState" and it returns the state when the value is greater 
or equal. If there is no such state it returns nullptr.

As far as I see the diagnostics are showing the proper path now..


Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:148
@@ +147,3 @@
+  QualType SubType = Cast->IgnoreParenImpCasts()->getType();
+
+  if (!CastType->isIntegerType() || !SubType->isIntegerType())

sorry about that, I have fixed it


http://reviews.llvm.org/D13126



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D13126: New static analyzer checker for loss of sign/precision

[Daniel Marjamäki via cfe-commits]

danielmarjamaki updated this revision to Diff 51402.
danielmarjamaki added a comment.

Minor fixes of review comments.

- Improved comments about the checker in the source code.
- Improved capitalization and punctuation in error messages.
- Renamed "canBe..." functions and changed their return type to ProgramStateRef 
to get better diagnostics.


http://reviews.llvm.org/D13126

Files:
  lib/StaticAnalyzer/Checkers/CMakeLists.txt
  lib/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
  test/Analysis/conversion.c

Index: test/Analysis/conversion.c
===
--- test/Analysis/conversion.c
+++ test/Analysis/conversion.c
@@ -0,0 +1,80 @@
+// RUN: %clang_cc1 -Wno-conversion -analyze -analyzer-checker=core,alpha.core.Conversion -verify %s
+
+unsigned char U8;
+signed char S8;
+
+void assign(unsigned U, signed S) {
+  if (S < -10)
+U8 = S; // expected-warning {{Loss of sign in implicit conversion}}
+  if (U > 300)
+S8 = U; // expected-warning {{Loss of precision in implicit conversion}}
+  if (S > 10)
+U8 = S;
+  if (U < 200)
+S8 = U;
+}
+
+void init1() {
+  long long A = 1LL << 60;
+  short X = A; // expected-warning {{Loss of precision in implicit conversion}}
+}
+
+void relational(unsigned U, signed S) {
+  if (S > 10) {
+if (U < S) {
+}
+  }
+  if (S < -10) {
+if (U < S) {
+} // expected-warning {{Loss of sign in implicit conversion}}
+  }
+}
+
+void multiplication(unsigned U, signed S) {
+  if (S > 5)
+S = U * S;
+  if (S < -10)
+S = U * S; // expected-warning {{Loss of sign}}
+}
+
+void division(unsigned U, signed S) {
+  if (S > 5)
+S = U / S;
+  if (S < -10)
+S = U / S; // expected-warning {{Loss of sign}}
+}
+
+void dontwarn1(unsigned U, signed S) {
+  U8 = S; // It might be known that S is always 0x00-0xff.
+  S8 = U; // It might be known that U is always 0x00-0xff.
+
+  U8 = -1;  // Explicit conversion.
+  S8 = ~0U; // Explicit conversion.
+  if (U > 300)
+U8 &= U; // No loss of precision since there is &=.
+}
+
+void dontwarn2(unsigned int U) {
+  if (U <= 4294967295) {
+  }
+  if (U <= (2147483647 * 2U + 1U)) {
+  }
+}
+
+void dontwarn3(int X) {
+  S8 = X ? 'a' : 'b';
+}
+
+// don't warn for macros
+#define DOSTUFF ({ unsigned X = 1000; U8 = X; })
+void dontwarn4() {
+  DOSTUFF;
+}
+
+// don't warn for calculations
+// seen some fp. For instance:  c2 = (c2 >= 'A' && c2 <= 'Z') ? c2 - 'A' + 'a' : c2;
+// there is a todo in the checker to handle calculations
+void dontwarn5() {
+  signed S = -32;
+  U8 = S + 10;
+}
Index: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
+++ lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
@@ -0,0 +1,181 @@
+//=== ConversionChecker.cpp -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// Check that there is not loss of sign/precision.
+//
+// ConversionChecker generates a subset of the warnings that are reported by
+// Wconversion. It is designed to be an alternative to Wconversion.
+//
+// ConversionChecker uses path sensitive analysis to avoid false positives. As
+// warnings are only reported when there is possible loss of sign/precision,
+// ConversionChecker is significantly less noisy than Wconversion.
+//
+//===--===//
+#include "ClangSACheckers.h"
+#include "clang/AST/ParentMap.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+class ConversionChecker : public Checker {
+public:
+  void checkPreStmt(const ImplicitCastExpr *Cast, CheckerContext ) const;
+
+private:
+  mutable std::unique_ptr BT;
+
+  void diagnoseLossOfPrecision(const ImplicitCastExpr *Cast,
+   CheckerContext ) const;
+  void diagnoseLossOfSign(const ImplicitCastExpr *Cast,
+  CheckerContext ) const;
+
+  void reportBug(CheckerContext , ProgramStateRef St, const char Msg[]) const;
+};
+}
+
+void ConversionChecker::checkPreStmt(const ImplicitCastExpr *Cast,
+ CheckerContext ) const {
+  // TODO: For now we only warn about DeclRefExpr, to avoid noise. Warn for
+  // calculations also.
+  if (!isa(Cast->IgnoreParenImpCasts()))
+return;
+
+  // Don't warn for loss of sign/precision in macros
+  if (Cast->getExprLoc().isMacroID())
+return;
+

Re: [PATCH] D13126: New static analyzer checker for loss of sign/precision

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a comment.

Here are some false positives I have marked... let me know if you want more...

I have marked this as a FP:

URL:  
ftp://ftp.se.debian.org/debian/pool/main/m/m17n-lib/m17n-lib_1.7.0.orig.tar.gz
File: m17n-lib-1.7.0/src/mtext.c
Line 1946

Code:

  int mtext_set_char (MText *mt, int pos, int c) {
  ...
  switch (mt->format)
  {
  case MTEXT_FORMAT_US_ASCII:
  mt->data[pos_unit] = c;  // <- WARNING
  break;
.
  }
  }

The precision of "c" can be too large for "mt->data[pos_unit]" if the format is 
not MTEXT_FORMAT_US_ASCII.

I believe it's a FP but it can probably be fixed by an assertion inside the 
"case".

I have marked this as a FP:

URL:  
ftp://ftp.se.debian.org/debian/pool/main/n/netkit-ftp/netkit-ftp_0.17.orig.tar.gz
File: netkit-ftp-0.17/ftp/ftp.c
Line 416

Code:

while ((c = getc(cin)) != '\n') {
  ...
  if (c == EOF) {
  
  return 4;
  }
  
  ...
  *cp++ = c;  // <- WARNING
  }

I have marked this as a FP:

URL:  ftp://ftp.se.debian.org/debian/pool/main/o/owl/owl_2.2.2.orig.tar.gz
File: owl-2.2.2.orig/keypress.c
Line: 185

Code:

  if (isascii(j)) {
kb2[0] = j;  // <- WARNING
kb2[1] = 0;
strcat(kb, kb2);
  }

If isascii() returns 1 then j is a 7-bit value. Then there is not loss of 
precision.


http://reviews.llvm.org/D13126



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D13126: New static analyzer checker for loss of sign/precision

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added inline comments.


Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:84
@@ +83,3 @@
+// Can E value be greater or equal than Val?
+static bool canBeGreaterEqual(CheckerContext , const Expr *E,
+  unsigned long long Val) {

danielmarjamaki wrote:
> zaks.anna wrote:
> > danielmarjamaki wrote:
> > > zaks.anna wrote:
> > > > This function returns true if the value "is" greater or equal, not "can 
> > > > be" greater or equal. The latter would be "return StGE".
> > > > 
> > > > Also, it's slightly better to return the StGE state and use it to 
> > > > report the bug. This way, our assumption is explicitly recorded in the 
> > > > error state.
> > > NoQ made the same comment. I disagree.
> > > 
> > > int A = 0;
> > > if (X) {
> > >  A = 1000;
> > > }
> > > U8 = A;  // <- Imho; A _can_ be 1000
> > > 
> > > Imho it's better to say that A _can_ be 1000 unless A is 1000 for all 
> > > possible execution paths through the code.
> > > 
> > > Do you still think "is" is better than "can be"?
> > The Clang Static Analyzer performs path sensitive analysis of the program. 
> > (It does not merge the paths at the "U8 = A" statement!!!) You will only be 
> > changing the state along a single execution path of this program. Along 
> > that path, A will always be 1000. 
> > 
> > When analyzing your example, the analyzer is going to separately analyze 2 
> > paths:
> > 1st path: A=0; X != 0; A =1000; U8 = A; // Here U8 is definitely 1000.
> > 2d  path: A=0; X == 0; U8 = A;   // Here U8 is definitely 0.
> > 
> > This video contains an intuitive explanation of symbolic execution 
> > technique we use: 
> > http://llvm.org/devmtg/2012-11/videos/Zaks-Rose-Checker24Hours.mp4
> I understand that and I still think that value of A "can be" 1000. Yes in 
> that path the value "is" 1000.
> 
> But as far as I see, you and others disagree with me. And therefore I will 
> change to "is".
For your information in Cppcheck I say that a value is "possible" if some 
path(s) generates that value. And "always" when all paths generate that value.

Code example:

int f(int x) {
  int a = 1000;
  int b = 0;
  if (x == 500)
a = 3;
  return a + b - x;
}

Debug output (cppcheck --debug-normal file.c):

##Value flow
Line 3
  1000 always 1000
Line 4
  0 always 0
Line 5
  x possible 500
  == possible 1
  500 always 500
Line 6
  3 always 3
Line 7
  a possible {1000,3}
  + possible {1000,3}
  b always 0
  x possible 500

For me personally it is confusing to say that A "is" 1000. That is different to 
how I normally think of it in Cppcheck.


http://reviews.llvm.org/D13126



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D13126: New static analyzer checker for loss of sign/precision

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added inline comments.


Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:84
@@ +83,3 @@
+// Can E value be greater or equal than Val?
+static bool canBeGreaterEqual(CheckerContext , const Expr *E,
+  unsigned long long Val) {

zaks.anna wrote:
> danielmarjamaki wrote:
> > zaks.anna wrote:
> > > This function returns true if the value "is" greater or equal, not "can 
> > > be" greater or equal. The latter would be "return StGE".
> > > 
> > > Also, it's slightly better to return the StGE state and use it to report 
> > > the bug. This way, our assumption is explicitly recorded in the error 
> > > state.
> > NoQ made the same comment. I disagree.
> > 
> > int A = 0;
> > if (X) {
> >  A = 1000;
> > }
> > U8 = A;  // <- Imho; A _can_ be 1000
> > 
> > Imho it's better to say that A _can_ be 1000 unless A is 1000 for all 
> > possible execution paths through the code.
> > 
> > Do you still think "is" is better than "can be"?
> The Clang Static Analyzer performs path sensitive analysis of the program. 
> (It does not merge the paths at the "U8 = A" statement!!!) You will only be 
> changing the state along a single execution path of this program. Along that 
> path, A will always be 1000. 
> 
> When analyzing your example, the analyzer is going to separately analyze 2 
> paths:
> 1st path: A=0; X != 0; A =1000; U8 = A; // Here U8 is definitely 1000.
> 2d  path: A=0; X == 0; U8 = A;   // Here U8 is definitely 0.
> 
> This video contains an intuitive explanation of symbolic execution technique 
> we use: http://llvm.org/devmtg/2012-11/videos/Zaks-Rose-Checker24Hours.mp4
I understand that and I still think that value of A "can be" 1000. Yes in that 
path the value "is" 1000.

But as far as I see, you and others disagree with me. And therefore I will 
change to "is".


http://reviews.llvm.org/D13126



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D13126: New static analyzer checker for loss of sign/precision

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added inline comments.


Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:84
@@ +83,3 @@
+// Can E value be greater or equal than Val?
+static bool canBeGreaterEqual(CheckerContext , const Expr *E,
+  unsigned long long Val) {

zaks.anna wrote:
> This function returns true if the value "is" greater or equal, not "can be" 
> greater or equal. The latter would be "return StGE".
> 
> Also, it's slightly better to return the StGE state and use it to report the 
> bug. This way, our assumption is explicitly recorded in the error state.
NoQ made the same comment. I disagree.

int A = 0;
if (X) {
 A = 1000;
}
U8 = A;  // <- Imho; A _can_ be 1000

Imho it's better to say that A _can_ be 1000 unless A is 1000 for all possible 
execution paths through the code.

Do you still think "is" is better than "can be"?


http://reviews.llvm.org/D13126



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D13126: New static analyzer checker for loss of sign/precision

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a comment.

In http://reviews.llvm.org/D13126#379306, @zaks.anna wrote:

> Why is there such a large jump in the number of warnings reported in the last 
> patch iteration?
>  It went from "1678 projects where scanned. In total I got 124 warnings" to 
> "In 2215 projects it found 875 warnings." Did the number of warnings in the 
> initial 1678 projects stay the same?


No I improved the check.

My previous patch was limited: I have limited this patch hoping that it will be 
easier to triage and review. Right now I only warn if there is assignment and 
RHS is a DeclRefExpr and only for loss of precision.

> Is it possible to take a look at the nature of the false positives, as per 
> NoQ's request above?


I'll try when I get some time.

> This checker would benefit greatly from explaining why the errors occur. For 
> example, where the values of variables are being constrained. Other checkers 
> use BugReporterVisitor to generate the rich diagnostic information. Dow you 
> have plans on how to accomplish that for this checker?


Yes that sounds like a good idea.. I''ll try to look at that.


http://reviews.llvm.org/D13126



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D13126: New static analyzer checker for loss of sign/precision

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a comment.

ping


http://reviews.llvm.org/D13126



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D15332: new clang-tidy checker readability-non-const-parameter

[Daniel Marjamäki via cfe-commits]

danielmarjamaki updated this revision to Diff 50705.
danielmarjamaki marked 3 inline comments as done.
danielmarjamaki added a comment.

Fixed review comments


http://reviews.llvm.org/D15332

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/NonConstParameterCheck.cpp
  clang-tidy/readability/NonConstParameterCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-non-const-parameter.rst
  test/clang-tidy/readability-non-const-parameter.cpp

Index: test/clang-tidy/readability-non-const-parameter.cpp
===
--- test/clang-tidy/readability-non-const-parameter.cpp
+++ test/clang-tidy/readability-non-const-parameter.cpp
@@ -0,0 +1,251 @@
+// RUN: %check_clang_tidy %s readability-non-const-parameter %t
+
+// Currently the checker only warns about pointer arguments.
+//
+// It can be defined both that the data is const and that the pointer is const,
+// the checker only checks if the data can be const-specified.
+//
+// It does not warn about pointers to records or function pointers.
+
+// Some external function where first argument is nonconst and second is const.
+char *strcpy1(char *dest, const char *src);
+unsigned my_strcpy(char *buf, const char *s);
+unsigned my_strlen(const char *buf);
+
+// CHECK-MESSAGES: :[[@LINE+1]]:29: warning: parameter 'last' can be const [readability-non-const-parameter]
+void warn1(int *first, int *last) {
+  // CHECK-FIXES: {{^}}void warn1(int *first, const int *last) {{{$}}
+  *first = 0;
+  if (first < last) {
+  } // <- last can be const
+}
+
+// TODO: warning should be written here
+void warn2(char *p) {
+  char buf[10];
+  strcpy1(buf, p);
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:19: warning: parameter 'p' can be const
+void assign1(int *p) {
+  // CHECK-FIXES: {{^}}void assign1(const int *p) {{{$}}
+  const int *q;
+  q = p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:19: warning: parameter 'p' can be const
+void assign2(int *p) {
+  // CHECK-FIXES: {{^}}void assign2(const int *p) {{{$}}
+  const int *q;
+  q = p + 1;
+}
+
+void assign3(int *p) {
+  *p = 0;
+}
+
+void assign4(int *p) {
+  *p += 2;
+}
+
+void assign5(char *p) {
+  p[0] = 0;
+}
+
+void assign6(int *p) {
+  int *q;
+  q = p++;
+}
+
+void assign7(char *p) {
+  char *a, *b;
+  a = b = p;
+}
+
+void assign8(char *a, char *b) {
+  char *x;
+  x = (a ? a : b);
+}
+
+void assign9(unsigned char *str, const unsigned int i) {
+  unsigned char *p;
+  for (p = str + i; *p;) {
+  }
+}
+
+void assign10(int *buf) {
+  int i, *p;
+  for (i = 0, p = buf; i < 10; i++, p++) {
+*p = 1;
+  }
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:17: warning: parameter 'p' can be const
+void init1(int *p) {
+  // CHECK-FIXES: {{^}}void init1(const int *p) {{{$}}
+  const int *q = p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:17: warning: parameter 'p' can be const
+void init2(int *p) {
+  // CHECK-FIXES: {{^}}void init2(const int *p) {{{$}}
+  const int *q = p + 1;
+}
+
+void init3(int *p) {
+  int *q = p;
+}
+
+void init4(float *p) {
+  int *q = (int *)p;
+}
+
+void init5(int *p) {
+  int *i = p ? p : 0;
+}
+
+void init6(int *p) {
+  int *a[] = {p, p, 0};
+}
+
+void init7(int *p, int x) {
+  for (int *q = p + x - 1; 0; q++)
+;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:18: warning: parameter 'p' can be const
+int return1(int *p) {
+  // CHECK-FIXES: {{^}}int return1(const int *p) {{{$}}
+  return *p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:25: warning: parameter 'p' can be const
+const int *return2(int *p) {
+  // CHECK-FIXES: {{^}}const int *return2(const int *p) {{{$}}
+  return p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:25: warning: parameter 'p' can be const
+const int *return3(int *p) {
+  // CHECK-FIXES: {{^}}const int *return3(const int *p) {{{$}}
+  return p + 1;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:27: warning: parameter 'p' can be const
+const char *return4(char *p) {
+  // CHECK-FIXES: {{^}}const char *return4(const char *p) {{{$}}
+  return p ? p : "";
+}
+
+char *return5(char *s) {
+  return s;
+}
+
+char *return6(char *s) {
+  return s + 1;
+}
+
+char *return7(char *a, char *b) {
+  return a ? a : b;
+}
+
+char return8(int *p) {
+  return ++(*p);
+}
+
+void dontwarn1(int *p) {
+  ++(*p);
+}
+
+void dontwarn2(int *p) {
+  (*p)++;
+}
+
+int dontwarn3(_Atomic(int) * p) {
+  return *p;
+}
+
+void callFunction1(char *p) {
+  strcpy1(p, "abc");
+}
+
+void callFunction2(char *p) {
+  strcpy1([0], "abc");
+}
+
+void callFunction3(char *p) {
+  strcpy1(p + 2, "abc");
+}
+
+char *callFunction4(char *p) {
+  return strcpy1(p, "abc");
+}
+
+unsigned callFunction5(char *buf) {
+  unsigned len = my_strlen(buf);
+  return len + my_strcpy(buf, "abc");
+}
+
+void f6(int **p);
+void callFunction6(int *p) { f6(); }
+
+typedef union { void *v; } t;
+void f7(t obj);
+void callFunction7(int *p) {
+  f7((t){p});
+}
+
+void f8(int );
+void callFunction8(int *p) {
+  f8(*p);
+}
+
+// Don't warn about nonconst