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 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] D24232: Wbitfiled-constant-conversion : allow ~0 , ~(1<

[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 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 &Result) 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
+// Licen

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] 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] 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] D24759: [RFC][StaticAnalyser] fix unreachable code false positives when block numbers mismatch

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a comment.

Thanks!

I somehow missed your answer in cfe-dev.

I will continue working on this and hopefully have a proper patch soon.


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] D24232: Wbitfiled-constant-conversion : allow ~0 , ~(1<

[Daniel Marjamäki via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL282156: Fix Wbitfield-constant-conversion false positives 
(authored by danielmarjamaki).

Changed prior to commit:
  https://reviews.llvm.org/D24232?vs=70325&id=72169#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D24232

Files:
  cfe/trunk/lib/Sema/SemaChecking.cpp
  cfe/trunk/test/Sema/constant-conversion.c

Index: cfe/trunk/test/Sema/constant-conversion.c
===
--- cfe/trunk/test/Sema/constant-conversion.c
+++ cfe/trunk/test/Sema/constant-conversion.c
@@ -69,7 +69,8 @@
unsigned int reserved:28;
} f;
 
-   f.twoBits1 = ~1; // expected-warning {{implicit truncation from 'int' 
to bitfield changes value from -2 to 2}}
+   f.twoBits1 = ~0; // no-warning
+   f.twoBits1 = ~1; // no-warning
f.twoBits2 = ~2; // expected-warning {{implicit truncation from 'int' 
to bitfield changes value from -3 to 1}}
f.twoBits1 &= ~1; // no-warning
f.twoBits2 &= ~2; // no-warning
@@ -114,14 +115,19 @@
   char array_init[] = { 255, 127, 128, 129, 0 };
 }
 
+#define A 1
+
 void test10() {
   struct S {
 unsigned a : 4;
   } s;
   s.a = -1;
   s.a = 15;
   s.a = -8;
+  s.a = ~0;
+  s.a = ~0U;
+  s.a = ~(1getOpcode() == UO_Minus)
-if (isa(UO->getSubExpr()))
-  OriginalWidth = Value.getMinSignedBits();
+  if (UO->getOpcode() == UO_Minus || UO->getOpcode() == UO_Not)
+OriginalWidth = Value.getMinSignedBits();
 
   if (OriginalWidth <= FieldWidth)
 return false;


Index: cfe/trunk/test/Sema/constant-conversion.c
===
--- cfe/trunk/test/Sema/constant-conversion.c
+++ cfe/trunk/test/Sema/constant-conversion.c
@@ -69,7 +69,8 @@
 		unsigned int reserved:28;
 	} f;
 
-	f.twoBits1 = ~1; // expected-warning {{implicit truncation from 'int' to bitfield changes value from -2 to 2}}
+	f.twoBits1 = ~0; // no-warning
+	f.twoBits1 = ~1; // no-warning
 	f.twoBits2 = ~2; // expected-warning {{implicit truncation from 'int' to bitfield changes value from -3 to 1}}
 	f.twoBits1 &= ~1; // no-warning
 	f.twoBits2 &= ~2; // no-warning
@@ -114,14 +115,19 @@
   char array_init[] = { 255, 127, 128, 129, 0 };
 }
 
+#define A 1
+
 void test10() {
   struct S {
 unsigned a : 4;
   } s;
   s.a = -1;
   s.a = 15;
   s.a = -8;
+  s.a = ~0;
+  s.a = ~0U;
+  s.a = ~(1getOpcode() == UO_Minus)
-if (isa(UO->getSubExpr()))
-  OriginalWidth = Value.getMinSignedBits();
+  if (UO->getOpcode() == UO_Minus || UO->getOpcode() == UO_Not)
+OriginalWidth = Value.getMinSignedBits();
 
   if (OriginalWidth <= FieldWidth)
 return false;
___
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] 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] 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

[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 &S, SourceLocation OpLoc,
+static void DiagnoseAddOrMulInShift(Sema &S, 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 

[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

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

[Daniel Marjamäki via cfe-commits]

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

Minor updates of testcase


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,17 @@
 }
   }
 }
+
+// Don't warn about unreachable VarDecl.
+void dostuff(int*A);
+void varDecl(int X) {
+  switch (X) {
+int A; // No warning here.
+  case 1:
+dostuff(&A);
+break;
+  case 2:
+dostuff(&A);
+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,17 @@
 }
   }
 }
+
+// Don't warn about unreachable VarDecl.
+void dostuff(int*A);
+void varDecl(int X) {
+  switch (X) {
+int A; // No warning here.
+  case 1:
+dostuff(&A);
+break;
+  case 2:
+dostuff(&A);
+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] 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

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] 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] 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(&A);
+break;
+  case 2:
+dostuff(&A);
+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(&A);
+break;
+  case 2:
+dostuff(&A);
+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] 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 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]

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&id=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(&A);
+break;
+  case 2:
+dostuff(&A);
+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(&A);
+break;
+  case 2:
+dostuff(&A);
+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] 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 &S, 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 &S, 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*BgetOpcodeStr();
+  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 &S, 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 '<<' has lower precedence than '

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] 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 &Result) 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 distribut

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

[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

[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

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 
mailto:daniel.marjam...@evidente.se>> 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] 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

[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 &S, 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 &S, 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*BgetOpcodeStr();
+  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 &S, 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
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include

[PATCH] D19586: Misleading Indentation check

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added inline comments.


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

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] 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] 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 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 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#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 &L, 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.

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 &L, 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 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, &isNew);
@@ -619,12 +631,12 @@
   }
 }
 
-void CoreEngine::enqueueEndOfFunction(ExplodedNodeSet &Set) {
+void CoreEngine::enqueueEndOfFunction(ExplodedNodeSet &Set, 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]

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&id=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 &Set);
+  void enqueueEndOfFunction(ExplodedNodeSet &Set, 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 added inline comments.



Comment at: test/Analysis/unreachable-code-path.c:201
+static int inlineFunction(const int i) {
+  if (table[i] != 0)
+return 1;

NoQ wrote:
> a.sidorin wrote:
> > I have a small question. Is it possible to simplify this sample with 
> > removing of table[] array? Like putting something like `i != 0` into 
> > condition. As I understand, the problem is not array-related.
> Any `UnknownVal` in the condition would trigger this issue.
> Is it possible to simplify this sample with removing of table[] array? 

I tried to simplify as much as possible. But as NoQ says an UnknownVal is 
required here for this test.


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#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] 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] 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 &Result) 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 @@
+//===--- RedundantDeclaratio

[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] 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] 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 &C) const {
+  CheckerContext &C) 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 &C) const {
+ CheckerContext &C) 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 &C) const {
+  CheckerContext &C) 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 &C) const {
+ CheckerContext &C) 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] 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

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 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(&p[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(&p); }
+
+typedef union { void *v; } t;
+void f7(t obj);
+void callFunction7(int *p) {
+  f7((t){p});
+}
+
+void f8(int &x);
+void callFunct

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(&p[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(&p); }
+
+typedef union { void *v; } t;
+void f7(t obj);
+void callFunction7(int *p) {
+  f7((t){p});
+}
+
+void f8(int &x);
+void callFunction8(int *p) {
+  f8(*p);

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&id=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 &Result) 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 "NonC

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 @@
+//===--- MisplacedArrayIndexCheck

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 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
+++ clang-tidy/readability/MisplacedArrayInd

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] 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] 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] 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&id=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 &Result) 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

[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 &Result) 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.
+  Finder->addMatcher(va

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

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 &Result) 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 &Result) {
+  const auto *VD = Result.Nodes.getNodeAs("Decl")

[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&id=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] 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] 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]

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&id=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]

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&id=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 &Result) {
+  const NamedDecl *D = Result.Nodes.getNodeAs("Decl");
+  const auto *Prev = D->getPreviousDecl();
+  if (!Prev)
+return;
+  if (Prev->getLocation() == D->getLocation())
+return;
+
+  const SourceManager &SM = *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 cl

Re: [PATCH] D12359: New warning -Wnonconst-parameter when a pointer parameter can be const

[Daniel Marjamäki via cfe-commits]

danielmarjamaki updated this revision to Diff 35482.
danielmarjamaki marked 9 inline comments as done.
danielmarjamaki added a comment.

With the previous patch there was much noise when building Clang. I have fixed 
many false positives with improved handling of C++ code. However I have not 
been able to properly handle templates yet.

With this patch, no -Wnonconst-parameter diagnostics are written for C++ code. 
I hope that it will be possible to fix the noise for C++ code later and enable 
this diagnostic for C++ code also.


http://reviews.llvm.org/D12359

Files:
  include/clang/AST/DeclBase.h
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Parse/Parser.h
  include/clang/Sema/Sema.h
  lib/Parse/ParseDeclCXX.cpp
  lib/Parse/ParseExpr.cpp
  lib/Parse/ParseExprCXX.cpp
  lib/Parse/ParseStmt.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaLambda.cpp
  lib/Sema/SemaOpenMP.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  lib/Serialization/ASTReaderDecl.cpp

Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -500,6 +500,7 @@
   D->setImplicit(Record[Idx++]);
   D->Used = Record[Idx++];
   D->setReferenced(Record[Idx++]);
+  D->setWritten();
   D->setTopLevelDeclInObjCContainer(Record[Idx++]);
   D->setAccess((AccessSpecifier)Record[Idx++]);
   D->FromASTFile = true;
Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -3631,6 +3631,8 @@
 if (OldVar->isUsed(false))
   NewVar->setIsUsed();
 NewVar->setReferenced(OldVar->isReferenced());
+if (OldVar->isWritten())
+  NewVar->setWritten();
   }
 
   InstantiateAttrs(TemplateArgs, OldVar, NewVar, LateAttrs, StartingScope);
Index: lib/Sema/SemaOpenMP.cpp
===
--- lib/Sema/SemaOpenMP.cpp
+++ lib/Sema/SemaOpenMP.cpp
@@ -482,6 +482,7 @@
  SourceLocation Loc,
  bool RefersToCapture = false) {
   D->setReferenced();
+  D->setWritten();
   D->markUsed(S.Context);
   return DeclRefExpr::Create(S.getASTContext(), NestedNameSpecifierLoc(),
  SourceLocation(), D, RefersToCapture, Loc, Ty,
Index: lib/Sema/SemaLambda.cpp
===
--- lib/Sema/SemaLambda.cpp
+++ lib/Sema/SemaLambda.cpp
@@ -814,7 +814,8 @@
   VarDecl *NewVD = VarDecl::Create(Context, CurContext, Loc,
Loc, Id, InitCaptureType, TSI, SC_Auto);
   NewVD->setInitCapture(true);
-  NewVD->setReferenced(true);
+  NewVD->setReferenced();
+  NewVD->setWritten();
   NewVD->markUsed(Context);
   NewVD->setInit(Init);
   return NewVD;
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -9525,6 +9525,17 @@
   }
 }
 
+/// \brief Mark symbols in l-value expression as written.
+static void MarkWritten(Expr *E) {
+  E = E->IgnoreParenCasts();
+  if (auto *D = dyn_cast(E))
+D->getDecl()->setWritten();
+  else if (auto *U = dyn_cast(E))
+MarkWritten(U->getSubExpr());
+  else if (auto *A = dyn_cast(E))
+MarkWritten(A->getBase());
+}
+
 // C99 6.5.16.1
 QualType Sema::CheckAssignmentOperands(Expr *LHSExpr, ExprResult &RHS,
SourceLocation Loc,
@@ -9535,6 +9546,8 @@
   if (CheckForModifiableLvalue(LHSExpr, Loc, *this))
 return QualType();
 
+  MarkWritten(LHSExpr);
+
   QualType LHSType = LHSExpr->getType();
   QualType RHSType = CompoundType.isNull() ? RHS.get()->getType() :
  CompoundType;
@@ -12818,7 +12831,8 @@
  
 CopyExpr = new (S.Context) DeclRefExpr(Var, RefersToCapturedVariable,
 DeclRefType, VK_LValue, Loc);
-Var->setReferenced(true);
+Var->setReferenced();
+Var->setWritten();
 Var->markUsed(S.Context);
   }
 
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -8931,6 +8931,23 @@
   }
 }
 
+static void MarkWritten(Expr *E) {
+  E = E->IgnoreParenCasts();
+  if (auto *B = dyn_cast(E)) {
+if (B->isAdditiveOp()) {
+  MarkWritten(B->getLHS());
+  MarkWritten(B->getRHS());
+}
+  } else if (auto *C = dyn_cast(E)) {
+MarkWritten(C->getFalseExpr());
+MarkWritten(C->getTrueExpr());
+  } else if (auto *D = dyn_cast(E)) {
+D->getDecl()->setWritten();
+  } else if (auto *U = dyn_cast(E)) {
+MarkWritten(U->getSubExpr());
+  }
+}
+
 /// AddInitializerToDecl - Adds the initializer Init to the
 /// declaration dcl.

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

[Daniel Marjamäki via cfe-commits]

danielmarjamaki created this revision.
danielmarjamaki added a reviewer: zaks.anna.
danielmarjamaki added a subscriber: cfe-commits.

This is a new static analyzer checker that warns when there is loss of sign and 
loss of precision.

It is similar in spirit to Wsign-compare/Wsign-conversion etc. But this checker 
uses proper analysis so the output is much more meaningful.

It has been tested on debian packages.

loss of precision results:
https://drive.google.com/file/d/0BykPmWrCOxt2WFV3NVhJdE94QUE/view
2195 projects, 1026 warnings

loss of sign results:
https://drive.google.com/file/d/0BykPmWrCOxt2cUpSQVlmUVJmR0k/view
2195 projects, 3613 warnigs

It seems to me that this checker shows that Clang does not properly track 
values. It seems to think that result of ! can be negative for instance.


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,65 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.core.Conversion -Wno-constant-conversion -verify %s
+
+unsigned char U8;
+signed char S8;
+
+void assign(unsigned U, signed S) {
+  if (S < -10)
+U8 = S; // expected-warning {{Implicit conversion changes signedness (negative value)}}
+  if (U > 300)
+S8 = U; // expected-warning {{Loss of precision, value too high}}
+  if (S > 10)
+U8 = S;
+  if (U < 200)
+S8 = U;
+}
+
+void relational(unsigned U, signed S) {
+  if (S > 10) {
+if (U < S) {}
+  }
+  if (S < -10) {
+if (U < S) {} // expected-warning {{Implicit conversion changes signedness (negative value)}}
+  }
+}
+
+void multiplication(unsigned U, signed S) {
+  if (S > 5)
+S = U * S;
+  if (S < -10)
+S = U * S; // expected-warning {{Implicit conversion changes signedness (negative value)}}
+}
+
+void division(unsigned U, signed S) {
+  if (S > 5)
+S = U / S;
+  if (S < -10)
+S = U / S; // expected-warning {{Implicit conversion changes signedness (negative value)}}
+}
+
+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(int i) {
+  static unsigned char Buf[200] = {200,100,200};
+  int S;
+  for (int i = 0; i < 200; i++) {
+S = Buf[i];  // RHS is smaller than LHS
+  }
+}
+
+void dontwarn3(unsigned int U) {
+  if (U <= 4294967295) {}
+  if (U <= (2147483647 * 2U + 1U)) {}
+}
+
+void dontwarn4(int X) {
+  S8 = X ? 'a' : 'b';
+}
Index: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
+++ lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
@@ -0,0 +1,169 @@
+//=== ConversionChecker.cpp -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// This defines ConversionChecker that warns about dangerous conversions where
+// there is possible loss of sign or loss of precision.
+//
+//===--===//
+#include "ClangSACheckers.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> {
+  mutable std::unique_ptr BT;
+
+public:
+  void checkPreStmt(const BinaryOperator *B, CheckerContext &C) const {
+BinaryOperator::Opcode Opc = B->getOpcode();
+if (Opc == BO_Assign || Opc == BO_MulAssign || Opc == BO_DivAssign) {
+  diagnoseLossOfSign(B, C);
+  diagnoseLossOfPrecision(B, C);
+} else if (B->isRelationalOp() || B->isMultiplicativeOp()) {
+  diagnoseLossOfSign(B, C);
+}
+  }
+
+private:
+  void diagnoseLossOfSign(const BinaryOperator *B, CheckerContext &C) const;
+  void diagnoseLossOfPrecision(const BinaryOperator *B,
+   CheckerContext &C) const;
+
+  void reportBug(CheckerContext &C, const char Msg[]) const {
+// Generate an error node.
+ExplodedNode *N = C.generateErrorNode(C.getState());
+if (!N)
+  return;
+
+if (!BT)
+  BT.reset(new BuiltinBug(this, "Conversion", "Loss of sign/precision."));
+
+// Generate a re

Re: [PATCH] D12359: New warning -Wnonconst-parameter when a pointer parameter can be const

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a comment.

Thanks! Very good comments.

I will look at the comments asap. but unfortunately I don't have time right now.

I expect that I can continue working on this warning in a few weeks.



Comment at: include/clang/AST/DeclBase.h:279
@@ +278,3 @@
+  /// be "const".
+  unsigned Written : 1;
+

aaron.ballman wrote:
> Should this bit be sunk all the way down into Decl? What does it mean for a 
> TypedefNameDecl or LabelDecl to be written? This seems like it belongs more 
> with something like VarDecl (but you might need FieldDecl for C++ support, so 
> perhaps ValueDecl?), but I'm not certain.
> 
> I'm still a bit confused by "written" in the name (here and with the 
> isWritten(), etc) -- It refers to is whether the declaration is used as a 
> non-const lvalue, not whether the variable is spelled out in code (as opposed 
> to an implicit variable, such as ones used by range-based for loops). Perhaps 
> HasNonConstUse, or something a bit more descriptive?
I agree. I will investigate. I only want to warn about parameters and nothing 
more but maybe vardecl is a good place.

Since you think 'NonConstUse' is better than 'Written' I will change.. I have 
no opinion.



Comment at: include/clang/AST/DeclBase.h:545
@@ -540,1 +544,3 @@
 
+  /// \brief Whether the declared symbol is written.
+  bool isWritten() const { return Written; }

aaron.ballman wrote:
> What does it mean for a declared symbol to be written? We have a 
> similar-sounding function in CXXCtorInitializer that means the initializer 
> was explicitly written, but I don't think the same applies here?
I have changed this to:

  /// \brief Whether the declared symbol is written either directly or
  /// indirectly. A "written" declaration can't be const.

Is this ok?


Comment at: lib/Parse/ParseStmt.cpp:379
@@ +378,3 @@
+  if (auto *B = dyn_cast(E)) {
+if (B->isAdditiveOp()) {
+  // p + 2

aaron.ballman wrote:
> Why does addition count as "writing?"
see dontwarn13 and dontwarn16. if taking the address "p" is a "write" then 
taking the address "p+2" is also a "write".


Comment at: lib/Parse/ParseStmt.cpp:401
@@ +400,3 @@
+  } else if (auto *C = dyn_cast(E)) {
+MarkWritten(C->getTrueExpr());
+MarkWritten(C->getFalseExpr());

aaron.ballman wrote:
> Again, why?
It's for code like dontwarn7 , dontwarn8, dontwarn9. If taking the address "p" 
is a "write" then "x?p:q" is a "write".


Comment at: lib/Sema/SemaExpr.cpp:9518
@@ -9517,1 +9517,3 @@
 
+// Mark symbols in l-value expression as written.
+static void MarkWritten(Expr *E) {

aaron.ballman wrote:
> Use \brief comments. Also, I don't see how this applies to lvalue expressions?
> 
> Also, this function is almost identical to the one in SemaDecl.cpp, except 
> for array subscripts. Why the differences?
> Also, I don't see how this applies to lvalue expressions?

I wanted to indicate that this function is for instance used for LHS in 
assignments but not RHS. For instance there is no "address is taken".

> Also, this function is almost identical to the one in SemaDecl.cpp, except 
> for array subscripts. Why the differences?

I think that would cause FN in a testcase. but I'll need to recompile to know..


Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:3616
@@ -3615,2 +3615,3 @@
 NewVar->setReferenced(OldVar->isReferenced());
+NewVar->setWritten();
   }

aaron.ballman wrote:
> Should this rely on OldVar->isWritten()?
Good catch. Yes I do think so, I will fix.


Comment at: lib/Serialization/ASTReaderDecl.cpp:503
@@ -502,2 +502,3 @@
   D->setReferenced(Record[Idx++]);
+  D->setWritten();
   D->setTopLevelDeclInObjCContainer(Record[Idx++]);

aaron.ballman wrote:
> Why don't we need to read this value from the serialized AST?
hmm.. I want to see a testcase. I am not sure how I use serialized ASTs.

I added setWritten() after every setReferenced() where it would not cause FN in 
my testcases.


Comment at: test/SemaCXX/warn-nonconst-parameter.cpp:1
@@ +1,2 @@
+// RUN: %clang_cc1 -fsyntax-only -Wnonconst-parameter -verify %s
+//

aaron.ballman wrote:
> Missing test coverage for the template cases you handled in code.
> 
> Also, I this doesn't warn in cases I would expect involving references, like:
> 
> void f(int &r) {
>   int i = r;
> }
hmm.. yes references could be interesting too. I only use Clang on C code so 
handling references is low priority for me. but maybe it's just a 5 minutes 
hack.



Comment at: test/SemaCXX/warn-nonconst-parameter.cpp:14
@@ +13,3 @@
+
+class Derived /* : public Base */ {
+public:

aaron.ballman wrote:
> Why is Base commented out?
mistake. will be fixed.


http://reviews.llvm.org/D12359



___
cf

Re: [PATCH] D11035: trivial patch, improve constness

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a comment.

ping. can somebody review?


http://reviews.llvm.org/D11035



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

[PATCH] D11940: don't diagnose -Wunused-parameter in virtual method or method that overrides base class method

[Daniel Marjamäki via cfe-commits]

danielmarjamaki created this revision.
danielmarjamaki added a reviewer: krememek.
danielmarjamaki added a subscriber: cfe-commits.

Don't diagnose -Wunused-parameter in methods that override other methods 
because the overridden methods might use the parameter

Don't diagnose -Wunused-parameter in virtual methods because these might be 
overriden by other methods that use the parameter.

Such diagnostics could be more accurately written if they are based on 
whole-program-analysis that establish if such parameter is unused in all 
methods.



http://reviews.llvm.org/D11940

Files:
  lib/Sema/SemaDecl.cpp
  test/SemaCXX/warn-unused-parameters.cpp

Index: test/SemaCXX/warn-unused-parameters.cpp
===
--- test/SemaCXX/warn-unused-parameters.cpp
+++ test/SemaCXX/warn-unused-parameters.cpp
@@ -32,3 +32,20 @@
   auto l = [&t...]() { return sizeof...(s); };
   return l();
 }
+
+// Don't diagnose virtual methods or methods that override base class
+// methods.
+class Base {
+public:
+  virtual void f(int x);
+};
+
+class Derived : public Base {
+public:
+  // Don't warn in overridden methods.
+  virtual void f(int x) {}
+
+  // Don't warn in virtual methods.
+  virtual void a(int x) {}
+};
+
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -10797,8 +10797,13 @@
 
 if (!FD->isInvalidDecl()) {
   // Don't diagnose unused parameters of defaulted or deleted functions.
-  if (!FD->isDeleted() && !FD->isDefaulted())
-DiagnoseUnusedParameters(FD->param_begin(), FD->param_end());
+  if (!FD->isDeleted() && !FD->isDefaulted()) {
+// Don't diagnose unused parameters in virtual methods or
+// in methods that override base class methods.
+const auto MD = dyn_cast(FD);
+if (!MD || (MD->size_overridden_methods() == 0U && !MD->isVirtual()))
+  DiagnoseUnusedParameters(FD->param_begin(), FD->param_end());
+  }
   DiagnoseSizeOfParametersAndReturnValue(FD->param_begin(), 
FD->param_end(),
  FD->getReturnType(), FD);
 


Index: test/SemaCXX/warn-unused-parameters.cpp
===
--- test/SemaCXX/warn-unused-parameters.cpp
+++ test/SemaCXX/warn-unused-parameters.cpp
@@ -32,3 +32,20 @@
   auto l = [&t...]() { return sizeof...(s); };
   return l();
 }
+
+// Don't diagnose virtual methods or methods that override base class
+// methods.
+class Base {
+public:
+  virtual void f(int x);
+};
+
+class Derived : public Base {
+public:
+  // Don't warn in overridden methods.
+  virtual void f(int x) {}
+
+  // Don't warn in virtual methods.
+  virtual void a(int x) {}
+};
+
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -10797,8 +10797,13 @@
 
 if (!FD->isInvalidDecl()) {
   // Don't diagnose unused parameters of defaulted or deleted functions.
-  if (!FD->isDeleted() && !FD->isDefaulted())
-DiagnoseUnusedParameters(FD->param_begin(), FD->param_end());
+  if (!FD->isDeleted() && !FD->isDefaulted()) {
+// Don't diagnose unused parameters in virtual methods or
+// in methods that override base class methods.
+const auto MD = dyn_cast(FD);
+if (!MD || (MD->size_overridden_methods() == 0U && !MD->isVirtual()))
+  DiagnoseUnusedParameters(FD->param_begin(), FD->param_end());
+  }
   DiagnoseSizeOfParametersAndReturnValue(FD->param_begin(), FD->param_end(),
  FD->getReturnType(), FD);
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D11940: don't diagnose -Wunused-parameter in virtual method or method that overrides base class method

[Daniel Marjamäki via cfe-commits]

danielmarjamaki abandoned this revision.
danielmarjamaki added a comment.

In http://reviews.llvm.org/D11940#221718, @thakis wrote:

> Can't you just change your signature to
>
>   virtual void a(int /* x */) {}
>   
>
> in these cases?


hmm.. yes I agree.

You are right, so I change my mind about this patch.


http://reviews.llvm.org/D11940



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

SV: [PATCH] D11940: don't diagnose -Wunused-parameter in virtual method or method that overrides base class method

[Daniel Marjamäki via cfe-commits]

ideally there should be no -Wunused-parameter compiler warning when the 
parameter is used.

would it feel better to move the "FP" warnings about virtual functions, for 
instance to clang-tidy?

> If you enable this warning, you probably want to know about unused 
> parameters, independent of if your function is virtual or not, no?

imho there are some compiler warnings that are too noisy. I don't like to get a 
warning when there is obviously no bug:

sign compare:
  if the signed value is obviously not negative then there is no bug:
signed x;  .. if (x>10 && x < s.size())
unused parameter:
  could check in the current translation unit if the parameter is used in an 
overloaded method.
constructor initialization order:
  should not warn if the order obviously does not matter. for instance 
initialization order of pod variables using constants.
etc

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: tha...@google.com [tha...@google.com] för Nico Weber 
[tha...@chromium.org]
Skickat: den 11 augusti 2015 20:50
Till: David Blaikie
Kopia: reviews+d11940+public+578c1335b27aa...@reviews.llvm.org; Daniel 
Marjamäki; cfe-commits@lists.llvm.org
Ämne: Re: [PATCH] D11940: don't diagnose -Wunused-parameter in virtual method 
or method that overrides base class method

On Tue, Aug 11, 2015 at 11:32 AM, David Blaikie 
mailto:dblai...@gmail.com>> wrote:


On Tue, Aug 11, 2015 at 8:46 AM, Nico Weber via cfe-commits 
mailto:cfe-commits@lists.llvm.org>> wrote:
Can't you just change your signature to

  virtual void a(int /* x */) {}

in these cases?

You could - does it add much value to do that, though?

If you enable this warning, you probably want to know about unused parameters, 
independent of if your function is virtual or not, no?

(perhaps it does - it means you express the intent that the parameter is not 
used and the compiler helps you check that (so that for the parameters you 
think /should/ be used (you haven't commented out their name but accidentally 
shadow or otherwise fail to reference, you still get a warning))

- David


On Tue, Aug 11, 2015 at 6:43 AM, Daniel Marjamäki 
mailto:cfe-commits@lists.llvm.org>> wrote:
danielmarjamaki created this revision.
danielmarjamaki added a reviewer: krememek.
danielmarjamaki added a subscriber: cfe-commits.

Don't diagnose -Wunused-parameter in methods that override other methods 
because the overridden methods might use the parameter

Don't diagnose -Wunused-parameter in virtual methods because these might be 
overriden by other methods that use the parameter.

Such diagnostics could be more accurately written if they are based on 
whole-program-analysis that establish if such parameter is unused in all 
methods.



http://reviews.llvm.org/D11940

Files:
  lib/Sema/SemaDecl.cpp
  test/SemaCXX/warn-unused-parameters.cpp

Index: test/SemaCXX/warn-unused-parameters.cpp
===
--- test/SemaCXX/warn-unused-parameters.cpp
+++ test/SemaCXX/warn-unused-parameters.cpp
@@ -32,3 +32,20 @@
   auto l = [&t...]() { return sizeof...(s); };
   return l();
 }
+
+// Don't diagnose virtual methods or methods that override base class
+// methods.
+class Base {
+public:
+  virtual void f(int x);
+};
+
+class Derived : public Base {
+public:
+  // Don't warn in overridden methods.
+  virtual void f(int x) {}
+
+  // Don't warn in virtual methods.
+  virtual void a(int x) {}
+};
+
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -10797,8 +10797,13 @@

 if (!FD->isInvalidDecl()) {
   // Don't diagnose unused parameters of defaulted or deleted functions.
-  if (!FD->isDeleted() && !FD->isDefaulted())
-DiagnoseUnusedParameters(FD->param_begin(), FD->param_end());
+  if (!FD->isDeleted() && !FD->isDefaulted()) {
+// Don't diagnose unused parameters in virtual methods or
+// in methods that override base class methods.
+const auto MD = dyn_cast(FD);
+if (!MD || (MD->size_overridden_methods() == 0U && !MD->isVirtual()))
+  DiagnoseUnusedParameters(FD->param_begin(), FD->param_end());
+  }
   DiagnoseSizeOfParametersAndReturnValue(FD->param_begin(), 
FD->param_end(),
  FD->getReturnType(), FD);




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



___
cfe-commits mailing list

SV: [PATCH] D11940: don't diagnose -Wunused-parameter in virtual method or method that overrides base class method

[Daniel Marjamäki via cfe-commits]

Hello!

> If the subset of cases where this warning fires on parameters to virtual 
> functions produces more noise (if a significant % of cases just result in 
> commenting out the parameter name rather than removing the parameter) than 
> signal, it's certainly within the realm of discussion that we have about 
> whether that subset of cases is worth keeping in the warning.

yes I agree.


> (And if you don't like this warning, then don't enable it.)

I want to have warnings when the parameter is unused. I want to enable only 
those.

Personally I would appreciate if there was conservative compiler warnings that 
I could enable for -Wunused-parameter, -Wsign-compare,  that only warns 
when the code is possibly unsafe or there is unused parameter etc.

I am not saying we must remove those other warnings. I am fine that they are 
enabled separately for instance -Wunused-parameter-virtual so I can disable 
them with -Wno-unused-parameter-virtual. Or move them to the analyser or 
clang-tidy or something.


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: tha...@google.com [tha...@google.com] för Nico Weber 
[tha...@chromium.org]
Skickat: den 12 augusti 2015 22:13
Till: David Blaikie
Kopia: Daniel Marjamäki; 
reviews+d11940+public+578c1335b27aa...@reviews.llvm.org; 
cfe-commits@lists.llvm.org
Ämne: Re: [PATCH] D11940: don't diagnose -Wunused-parameter in virtual method 
or method that overrides base class method

On Wed, Aug 12, 2015 at 11:16 AM, David Blaikie 
mailto:dblai...@gmail.com>> wrote:


On Wed, Aug 12, 2015 at 11:12 AM, Nico Weber 
mailto:tha...@chromium.org>> wrote:
On Wed, Aug 12, 2015 at 11:06 AM, David Blaikie 
mailto:dblai...@gmail.com>> wrote:


On Wed, Aug 12, 2015 at 10:11 AM, Nico Weber 
mailto:tha...@chromium.org>> wrote:
On Tue, Aug 11, 2015 at 10:15 PM, Daniel Marjamäki 
mailto:daniel.marjam...@evidente.se>> wrote:

ideally there should be no -Wunused-parameter compiler warning when the 
parameter is used.

would it feel better to move the "FP" warnings about virtual functions, for 
instance to clang-tidy?

> If you enable this warning, you probably want to know about unused 
> parameters, independent of if your function is virtual or not, no?

imho there are some compiler warnings that are too noisy. I don't like to get a 
warning when there is obviously no bug:

sign compare:
  if the signed value is obviously not negative then there is no bug:
signed x;  .. if (x>10 && x < s.size())
unused parameter:
  could check in the current translation unit if the parameter is used in an 
overloaded method.

It doesn't warn about the presence of the parameter, but about the presence of 
the name. If you say f(int, int) instead of f(int a, int b) then the warning 
won't fire.

(And if you don't like this warning, then don't enable it.)

This isn't usually the approach we take with Clang's warnings - we try to 
remove false positives (where "false positive" is usually defined as "diagnoses 
something which is not a bug" (where bug is defined as "the resulting program 
behaves in a way that the user doesn't intend/expect")) where practical.

Sure, for warnings that are supposed to find bugs. The -Wunused warnings warn 
about stuff that's unused, not bugs.

Seems a reasonable analog here, though, would be that a true positive for 
-Wunused is when the thing really is unused and should be removed. Commenting 
out the variable name is the suppression mechanism to workaround false 
positives.

I disagree with this assessment. The warning warns about unused parameter 
names. So this is a true positive.

If there's a targetable subset of cases where the s/n is low enough, it could 
be reasonable to suppress the warning in that subset, I think.

(see improvements to -Wunreachable-code to suppress cases that are unreachable 
in this build (sizeof(int) == 4 conditions, macros, etc), or represent valid 
defensive programming (default in a covered enum switch) to make the diagnostic 
more useful/less noisy)



If the subset of cases where this warning fires on parameters to virtual 
functions produces more noise (if a significant % of cases just result in 
commenting out the parameter name rather than removing the parameter) than 
signal, it's certainly within the realm of discussion that we have about 
whether that subset of cases is worth keeping in the warning.

- David


constructor initialization order:
  should not warn if the order obviously does not matter. for instance 
initialization order of pod variables using constants.
etc

Best regards,
Daniel Marjamäki

.

[PATCH] D12359: New warning -Wnonconst-parameter when a pointer parameter can be const

[Daniel Marjamäki via cfe-commits]

danielmarjamaki created this revision.
danielmarjamaki added a subscriber: cfe-commits.

This is a new warning for Clang. It will warn when a pointer parameter can be 
const.

The design is inspired by the Wunused-parameter warning.

I have tested this on many debian packages. In 2151 projects there were 60834 
warnings. So in average there were ~30 warnings / project. Most of my 
"dontwarn" testcases are inspired by false positives I have seen and fixed. The 
false positive ratio is imho not bad right now but I will continue to look in 
the log to see if there are more false positives.


http://reviews.llvm.org/D12359

Files:
  include/clang/AST/DeclBase.h
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/Parse/ParseExpr.cpp
  lib/Parse/ParseStmt.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaLambda.cpp
  lib/Sema/SemaOpenMP.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  lib/Serialization/ASTReaderDecl.cpp
  test/Sema/warn-nonconst-parameter.c
  test/SemaCXX/warn-nonconst-parameter.cpp

Index: test/SemaCXX/warn-nonconst-parameter.cpp
===
--- test/SemaCXX/warn-nonconst-parameter.cpp
+++ test/SemaCXX/warn-nonconst-parameter.cpp
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 -fsyntax-only -Wnonconst-parameter -verify %s
+//
+// Test that -Wnonconst-parameter does not warn for virtual functions.
+//
+
+// expected-no-diagnostics
+
+class Base {
+public:
+  virtual int f(int*p);
+};
+
+
+class Derived /* : public Base */ {
+public:
+  virtual int f(int*p){return 0;}
+};
Index: test/Sema/warn-nonconst-parameter.c
===
--- test/Sema/warn-nonconst-parameter.c
+++ test/Sema/warn-nonconst-parameter.c
@@ -0,0 +1,89 @@
+// RUN: %clang_cc1 -fsyntax-only -Wnonconst-parameter -verify %s
+
+// Currently the -Wnonconst-parameter only warns about pointer arguments.
+//
+// It can be defined both that the data is const and that the pointer is const,
+// the -Wnonconst-parameter 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.
+void strcpy1(char *dest, const char *src);
+
+
+int warn1(int *p) { // expected-warning {{parameter 'p' can be const}}
+  return *p;
+}
+
+void warn2(int *first, int *last) { // expected-warning {{parameter 'last' can be const}}
+  *first = 0;
+  if (first < last) {} // <- last can be const
+}
+
+void warn3(char *p) { // expected-warning {{parameter 'p' can be const}}
+  char buf[10];
+  strcpy1(buf, p);
+}
+
+int dontwarn1(const int *p) {
+  return *p;
+}
+
+void dontwarn2(int *p) {
+  *p = 0;
+}
+
+void dontwarn3(char *p) {
+  p[0] = 0;
+}
+
+void dontwarn4(int *p) {
+  int *q = p;
+  *q = 0;
+}
+
+void dontwarn5(float *p) {
+  int *q = (int *)p;
+}
+
+void dontwarn6(char *p) {
+  char *a, *b;
+  a = b = p;
+}
+
+void dontwarn7(int *p) {
+  int *i = p ? p : 0;
+}
+
+void dontwarn8(char *p) {
+  char *x;
+  x = (p ? p : "");
+}
+
+void dontwarn9(int *p) {
+  ++(*p);
+}
+
+void dontwarn10(unsigned char *str, const unsigned int i) {
+unsigned char *p;
+for (p = str + i; *p; ) {}
+}
+
+void dontwarn11(char *p) {
+  strcpy1(p, "abc");
+}
+
+void dontwarn12(char *p) {
+  strcpy1(p+2, "abc");
+}
+
+// Don't warn about nonconst function pointers that can be const.
+void functionpointer(double f(double), int x) {
+  f(x);
+}
+
+// Don't warn about nonconst record pointers that can be const.
+struct XY { int x; int y; };
+int recordpointer(struct XY *xy) {
+  return xy->x;
+}
Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -500,6 +500,7 @@
   D->setImplicit(Record[Idx++]);
   D->Used = Record[Idx++];
   D->setReferenced(Record[Idx++]);
+  D->setWritten();
   D->setTopLevelDeclInObjCContainer(Record[Idx++]);
   D->setAccess((AccessSpecifier)Record[Idx++]);
   D->FromASTFile = true;
Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -3613,6 +3613,7 @@
 if (OldVar->isUsed(false))
   NewVar->setIsUsed();
 NewVar->setReferenced(OldVar->isReferenced());
+NewVar->setWritten();
   }
 
   // See if the old variable had a type-specifier that defined an anonymous tag.
Index: lib/Sema/SemaOpenMP.cpp
===
--- lib/Sema/SemaOpenMP.cpp
+++ lib/Sema/SemaOpenMP.cpp
@@ -466,6 +466,7 @@
  SourceLocation Loc,
  bool RefersToCapture = false) {
   D->setReferenced();
+  D->setWritten();
   D->markUsed(S.Context);
   ret

Re: [PATCH] D12359: New warning -Wnonconst-parameter when a pointer parameter can be const

[Daniel Marjamäki via cfe-commits]

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

Thanks! I fixed that FP.


http://reviews.llvm.org/D12359

Files:
  include/clang/AST/DeclBase.h
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Parse/Parser.h
  include/clang/Sema/Sema.h
  lib/Parse/ParseExpr.cpp
  lib/Parse/ParseStmt.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaLambda.cpp
  lib/Sema/SemaOpenMP.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  lib/Serialization/ASTReaderDecl.cpp
  test/Sema/warn-nonconst-parameter.c
  test/SemaCXX/warn-nonconst-parameter.cpp

Index: test/SemaCXX/warn-nonconst-parameter.cpp
===
--- test/SemaCXX/warn-nonconst-parameter.cpp
+++ test/SemaCXX/warn-nonconst-parameter.cpp
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 -fsyntax-only -Wnonconst-parameter -verify %s
+//
+// Test that -Wnonconst-parameter does not warn for virtual functions.
+//
+
+// expected-no-diagnostics
+
+class Base {
+public:
+  virtual int f(int*p);
+};
+
+
+class Derived /* : public Base */ {
+public:
+  virtual int f(int*p){return 0;}
+};
Index: test/Sema/warn-nonconst-parameter.c
===
--- test/Sema/warn-nonconst-parameter.c
+++ test/Sema/warn-nonconst-parameter.c
@@ -0,0 +1,102 @@
+// RUN: %clang_cc1 -fsyntax-only -Wnonconst-parameter -verify %s
+
+// Currently the -Wnonconst-parameter only warns about pointer arguments.
+//
+// It can be defined both that the data is const and that the pointer is const,
+// the -Wnonconst-parameter 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);
+
+
+int warn1(int *p) { // expected-warning {{parameter 'p' can be const}}
+  return *p;
+}
+
+void warn2(int *first, int *last) { // expected-warning {{parameter 'last' can be const}}
+  *first = 0;
+  if (first < last) {} // <- last can be const
+}
+
+void warn3(char *p) { // expected-warning {{parameter 'p' can be const}}
+  char buf[10];
+  strcpy1(buf, p);
+}
+
+int dontwarn1(const int *p) {
+  return *p;
+}
+
+void dontwarn2(int *p) {
+  *p = 0;
+}
+
+void dontwarn3(char *p) {
+  p[0] = 0;
+}
+
+void dontwarn4(int *p) {
+  int *q = p;
+  *q = 0;
+}
+
+void dontwarn5(float *p) {
+  int *q = (int *)p;
+}
+
+void dontwarn6(char *p) {
+  char *a, *b;
+  a = b = p;
+}
+
+void dontwarn7(int *p) {
+  int *i = p ? p : 0;
+}
+
+void dontwarn8(char *p) {
+  char *x;
+  x = (p ? p : "");
+}
+
+char *dontwarn9(char *p) {
+  char *x;
+  return p ? p : "";
+}
+
+void dontwarn10(int *p) {
+  ++(*p);
+}
+
+char dontwarn11(int *p) {
+  return ++(*p);
+}
+
+void dontwarn12(unsigned char *str, const unsigned int i) {
+unsigned char *p;
+for (p = str + i; *p; ) {}
+}
+
+void dontwarn13(char *p) {
+  strcpy1(p, "abc");
+}
+
+void dontwarn14(char *p) {
+  strcpy1(p+2, "abc");
+}
+
+char *dontwarn15(char *p) {
+  return strcpy1(p, "abc");
+}
+
+// Don't warn about nonconst function pointers that can be const.
+void functionpointer(double f(double), int x) {
+  f(x);
+}
+
+// Don't warn about nonconst record pointers that can be const.
+struct XY { int x; int y; };
+int recordpointer(struct XY *xy) {
+  return xy->x;
+}
Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -500,6 +500,7 @@
   D->setImplicit(Record[Idx++]);
   D->Used = Record[Idx++];
   D->setReferenced(Record[Idx++]);
+  D->setWritten();
   D->setTopLevelDeclInObjCContainer(Record[Idx++]);
   D->setAccess((AccessSpecifier)Record[Idx++]);
   D->FromASTFile = true;
Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -3613,6 +3613,7 @@
 if (OldVar->isUsed(false))
   NewVar->setIsUsed();
 NewVar->setReferenced(OldVar->isReferenced());
+NewVar->setWritten();
   }
 
   // See if the old variable had a type-specifier that defined an anonymous tag.
Index: lib/Sema/SemaOpenMP.cpp
===
--- lib/Sema/SemaOpenMP.cpp
+++ lib/Sema/SemaOpenMP.cpp
@@ -466,6 +466,7 @@
  SourceLocation Loc,
  bool RefersToCapture = false) {
   D->setReferenced();
+  D->setWritten();
   D->markUsed(S.Context);
   return DeclRefExpr::Create(S.getASTContext(), NestedNameSpecifierLoc(),
  SourceLocation(), D, RefersToCapture, Loc, Ty,
Index: lib/Sema/SemaLambda.cpp
===
--- lib/Sema/SemaLambda

Re: [PATCH] D12359: New warning -Wnonconst-parameter when a pointer parameter can be const

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a comment.

I saw a FP when I looked through the logs today...

Example code:

  void dostuff(int *);
  
  void f(int *p) {
  #ifdef X
  dostuff(p);
  #endif
  
  if (*p == 0) {}
  }

As far as I know there is nothing I can do about this in the checker.

A user could for instance solve this with a macro:

  void dostuff(int *);
  
  #define NONCONST(p)   (void)(0 ? 0 : ++(*p))
  
  void f(int *p) {
  NONCONST(p);
  
  #ifdef X
  dostuff(p);
  #endif
  
  if (*p == 0) {}
  }

Or by using a second pointer:

  void dostuff(int *);
  void f(int *p) {
  int *p2 = p;
  
  #ifdef X
  dostuff(*p2);
  #endif
  
  if (*p2 == 0) {}
  }


http://reviews.llvm.org/D12359



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

Re: [PATCH] D12359: New warning -Wnonconst-parameter when a pointer parameter can be const

[Daniel Marjamäki via cfe-commits]

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

Fixed 2 false positives found in python source code.


http://reviews.llvm.org/D12359

Files:
  include/clang/AST/DeclBase.h
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Parse/Parser.h
  include/clang/Sema/Sema.h
  lib/Parse/ParseExpr.cpp
  lib/Parse/ParseStmt.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaLambda.cpp
  lib/Sema/SemaOpenMP.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  lib/Serialization/ASTReaderDecl.cpp
  test/Sema/warn-nonconst-parameter.c
  test/SemaCXX/warn-nonconst-parameter.cpp

Index: test/SemaCXX/warn-nonconst-parameter.cpp
===
--- test/SemaCXX/warn-nonconst-parameter.cpp
+++ test/SemaCXX/warn-nonconst-parameter.cpp
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 -fsyntax-only -Wnonconst-parameter -verify %s
+//
+// Test that -Wnonconst-parameter does not warn for virtual functions.
+//
+
+// expected-no-diagnostics
+
+class Base {
+public:
+  virtual int f(int*p);
+};
+
+
+class Derived /* : public Base */ {
+public:
+  virtual int f(int*p){return 0;}
+};
Index: test/Sema/warn-nonconst-parameter.c
===
--- test/Sema/warn-nonconst-parameter.c
+++ test/Sema/warn-nonconst-parameter.c
@@ -0,0 +1,113 @@
+// RUN: %clang_cc1 -fsyntax-only -Wnonconst-parameter -verify %s
+
+// Currently the -Wnonconst-parameter only warns about pointer arguments.
+//
+// It can be defined both that the data is const and that the pointer is const,
+// the -Wnonconst-parameter 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);
+
+
+int warn1(int *p) { // expected-warning {{parameter 'p' can be const}}
+  return *p;
+}
+
+void warn2(int *first, int *last) { // expected-warning {{parameter 'last' can be const}}
+  *first = 0;
+  if (first < last) {} // <- last can be const
+}
+
+void warn3(char *p) { // expected-warning {{parameter 'p' can be const}}
+  char buf[10];
+  strcpy1(buf, p);
+}
+
+int dontwarn1(const int *p) {
+  return *p;
+}
+
+void dontwarn2(int *p) {
+  *p = 0;
+}
+
+void dontwarn3(char *p) {
+  p[0] = 0;
+}
+
+void dontwarn4(int *p) {
+  int *q = p;
+  *q = 0;
+}
+
+void dontwarn5(float *p) {
+  int *q = (int *)p;
+}
+
+void dontwarn6(char *p) {
+  char *a, *b;
+  a = b = p;
+}
+
+void dontwarn7(int *p) {
+  int *i = p ? p : 0;
+}
+
+void dontwarn8(char *p) {
+  char *x;
+  x = (p ? p : "");
+}
+
+char *dontwarn9(char *p) {
+  char *x;
+  return p ? p : "";
+}
+
+void dontwarn10(int *p) {
+  ++(*p);
+}
+
+char dontwarn11(int *p) {
+  return ++(*p);
+}
+
+char *dontwarn12(char *s) {
+  return s;
+}
+
+void dontwarn13(unsigned char *str, const unsigned int i) {
+unsigned char *p;
+for (p = str + i; *p; ) {}
+}
+
+void dontwarn14(int *buf) {
+  int i, *p;
+  for (i=0, p=buf; i<10; i++, p++) {
+*p = 1;
+  }
+}
+
+void dontwarn15(char *p) {
+  strcpy1(p, "abc");
+}
+
+void dontwarn16(char *p) {
+  strcpy1(p+2, "abc");
+}
+
+char *dontwarn17(char *p) {
+  return strcpy1(p, "abc");
+}
+
+// Don't warn about nonconst function pointers that can be const.
+void functionpointer(double f(double), int x) {
+  f(x);
+}
+
+// Don't warn about nonconst record pointers that can be const.
+struct XY { int x; int y; };
+int recordpointer(struct XY *xy) {
+  return xy->x;
+}
Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -500,6 +500,7 @@
   D->setImplicit(Record[Idx++]);
   D->Used = Record[Idx++];
   D->setReferenced(Record[Idx++]);
+  D->setWritten();
   D->setTopLevelDeclInObjCContainer(Record[Idx++]);
   D->setAccess((AccessSpecifier)Record[Idx++]);
   D->FromASTFile = true;
Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -3613,6 +3613,7 @@
 if (OldVar->isUsed(false))
   NewVar->setIsUsed();
 NewVar->setReferenced(OldVar->isReferenced());
+NewVar->setWritten();
   }
 
   // See if the old variable had a type-specifier that defined an anonymous tag.
Index: lib/Sema/SemaOpenMP.cpp
===
--- lib/Sema/SemaOpenMP.cpp
+++ lib/Sema/SemaOpenMP.cpp
@@ -466,6 +466,7 @@
  SourceLocation Loc,
  bool RefersToCapture = false) {
   D->setReferenced();
+  D->setWritten();
   D->markUsed(S.Context);
   return DeclRefExpr::Create(S.getASTContext(), NestedNameSpecifierLoc(),

Re: [PATCH] D12359: New warning -Wnonconst-parameter when a pointer parameter can be const

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a comment.

In http://reviews.llvm.org/D12359#236334, @aaron.ballman wrote:

> I have concerns about this being a frontend warning. The true positive rate 
> seems rather high given the benign nature of the diagnostic, and the false 
> negative rate is quite high. This seems like it would make more sense as a 
> path-sensitive static analyzer warning instead of a frontend warning, as that 
> would justify the slightly high true positive rate, and rectify quite a bit 
> of the false negative rate.


I don't understand. The checker is path sensitive, isn't it? Do you see some 
problem that I don't?

It will warn if there is no write anywhere in the function. Except as I wrote, 
for some cases where #ifdef is used, but moving it to static analysis won't 
help.

> Have you tried running this over the Clang and LLVM code bases? How many 
> diagnostics does it produce?


Not yet. I'll do that.



Comment at: test/Sema/warn-nonconst-parameter.c:8
@@ +7,3 @@
+//
+// It does not warn about pointers to records or function pointers.
+

aaron.ballman wrote:
> How does it handle cases like:
> 
> void g(int);
> void f(volatile int *p) {
>   int j = *p; // Should not warn
>   int i = p[0]; // Should not warn
>   g(*p); // Should not warn
> }
> 
> void h(int *p) {
>   int i = p ? *p : 0; // Should warn
> }
> 
ok interesting. I have never seen a volatile pointer argument before. but 
technically I believe we should warn about f(). the function only reads p. 
maybe for stylistic reasons it would look weird to say that it's both volatile 
and const, is that why we should not warn?


http://reviews.llvm.org/D12359



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

RE: [PATCH] D12359: New warning -Wnonconst-parameter when a pointer parameter can be const

[Daniel Marjamäki via cfe-commits]

Hello!

> The checker isn't currently path sensitive as it doesn't pay attention
> to control flow graphs or how pointer values flow through a function
> body. I suppose this is a matter of scope more than anything; I see a
> logical extension of this functionality being with local variables as
> well as parameters. So, for instance:
>
>void f(int *p) {
>  int *i = p;
>  std::cout << *i;
>}

Imho that path analysis is not very interesting. The "p" can't be const until 
we see that "i" is const.

> This is true, for the current design of the patch, static analysis is
> less useful because the path sensitivity doesn't matter. But that also
> suggests if we wanted to add such a thing to the static analyzer
> someday, we'd have two ways of getting the same information if we
> stuck this in the frontend. It seems more logical to me to set this up
> as a static analysis checker so that we can extend it to be path
> sensitive under the same flag.

I wish we would have had this discussion earlier. Now I am not eager to rewrite 
it. for information this design has passed dev without comments:
http://lists.llvm.org/pipermail/cfe-dev/2015-August/044547.html

do you want that Wunused-parameter is moved from the frontend too? otherwise 
there will be similar path-sensitive analysis in the frontend anyway.

if we talk about the user interface.. imho it would be nice that this is a 
compiler warning. the analysis is quick and there should be little noise.

> Btw, since I forgot to mention it before, I think this is a great idea
> in general, thank you for working on it! :-)

Thanks! This is appreciated.

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: Aaron Ballman [aaron.ball...@gmail.com]
Skickat: den 31 augusti 2015 21:20
Till: reviews+d12359+public+799c9f67cbbb7...@reviews.llvm.org
Kopia: Daniel Marjamäki; stephan.bergmann.second...@googlemail.com; cfe-commits
Ämne: Re: [PATCH] D12359: New warning -Wnonconst-parameter when a pointer 
parameter can be const

On Mon, Aug 31, 2015 at 2:58 PM, Daniel Marjamäki
 wrote:
> danielmarjamaki added a comment.
>
> In http://reviews.llvm.org/D12359#236334, @aaron.ballman wrote:
>
>> I have concerns about this being a frontend warning. The true positive rate 
>> seems rather high given the benign nature of the diagnostic, and the false 
>> negative rate is quite high. This seems like it would make more sense as a 
>> path-sensitive static analyzer warning instead of a frontend warning, as 
>> that would justify the slightly high true positive rate, and rectify quite a 
>> bit of the false negative rate.
>
>
> I don't understand. The checker is path sensitive, isn't it? Do you see some 
> problem that I don't?

> The checker isn't currently path sensitive as it doesn't pay attention
> to control flow graphs or how pointer values flow through a function
> body. I suppose this is a matter of scope more than anything; I see a
> logical extension of this functionality being with local variables as
> well as parameters. So, for instance:

void f(int *p) {
  int *i = p;
  std::cout << *i;
}

I think code like the above should tell the user that both p and i can be const.

> It will warn if there is no write anywhere in the function. Except as I 
> wrote, for some cases where #ifdef is used, but moving it to static analysis 
> won't help.

This is true, for the current design of the patch, static analysis is
less useful because the path sensitivity doesn't matter. But that also
suggests if we wanted to add such a thing to the static analyzer
someday, we'd have two ways of getting the same information if we
stuck this in the frontend. It seems more logical to me to set this up
as a static analysis checker so that we can extend it to be path
sensitive under the same flag.

>
>> Have you tried running this over the Clang and LLVM code bases? How many 
>> diagnostics does it produce?
>
>
> Not yet. I'll do that.
>
>
> 
> Comment at: test/Sema/warn-nonconst-parameter.c:8
> @@ +7,3 @@
> +//
> +// It does not warn about pointers to records or function pointers.
> +
> 
> aaron.ballman wrote:
>> How does it handle cases like:
>>
>> void g(int);
>> void f(volatile int *p) {
>>   int j = *p; // Should not warn
>>   int i = p[0]; // Should not warn
>>   g(*p); // Should not warn
>> }
>>
>> void h(int *p) {
>>   int i = p ? *p : 0; // Should warn
>> }
>>
> ok interesting. I have never seen a volatile pointer argument before. but 
> technically I believe we should warn about f(). the function only reads p. 
> maybe for stylistic reasons it would look weird to say that it's both 
> volatile and const, is that why w

Re: [PATCH] D12359: New warning -Wnonconst-parameter when a pointer parameter can be const

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a comment.

> > > The checker isn't currently path sensitive as it doesn't pay attention

> 

> > 

> 

> > >  to control flow graphs or how pointer values flow through a function

> 

> > 

> 

> > >  body. I suppose this is a matter of scope more than anything; I see a

> 

> > 

> 

> > >  logical extension of this functionality being with local variables as

> 

> > 

> 

> > >  well as parameters. So, for instance:

> 

> > 

> 

> > > 

> 

> > 

> 

> > > void f(int *p) {

> 

> > 

> 

> > > 

> 

> > 

> 

> > >   int *i = p;

> 

> > 

> 

> > >   std::cout << *i;

> 

> > 

> 

> > > 

> 

> > 

> 

> > > }

> 

> > 

> 

> > 

> 

> > Imho that path analysis is not very interesting. The "p" can't be const 
> > until we see that "i" is const.

> 

> 

> Correct, but from the user's perspective, why are we not telling them

>  both can be const?


We want to have simple and clear warning messages. I will currently just write 
"parameter p can be const." Imho that is simple and clear. In your example I 
believe it would be required with a more complex message. Because p can't be 
const. It can only be const if i is made const first.

As I see it "my" analysis does not have any false negatives that would be 
avoided. It's just that 2 separate simple messages are clumped together into 1 
more complex message.

I also believe that solving this iteratively in small steps is less error prone.

> > if we talk about the user interface.. imho it would be nice that this is a 
> > compiler warning. the analysis is quick and there should be little noise.

> 

> 

> I'm not certain about the performance of the analysis (I suspect it's

>  relatively cheap though), but we do not usually want off-by-default

>  warnings in the frontend, and I suspect that this analysis would have

>  to be off by default due to the chattiness on well-formed code.


hmm.. I believe that this is common practice. I can see that people want to 
turn it off for legacy code though.

but we can look at the warnings on real code and discuss those. I have results 
from Clang.



Comment at: lib/Sema/SemaDecl.cpp:10334
@@ +10333,3 @@
+  continue;
+// To start with we don't warn about structs.
+if (T->getPointeeType().getTypePtr()->isRecordType())

aaron.ballman wrote:
> This seems *really* limiting (especially for C++ code), why the restriction?
2 reasons...

1. I don't think we should warn for structs whenever it's possible to make them 
const.

Example code:

struct FRED { char *str; };
void f(struct FRED *fred) {
strcpy(fred->str, "abc");
}

fred can be const but imho we should not warn about this. Imho it would be 
misleading to make fred const.

2. I wanted to keep the scope of this checker limited to start with. If we want 
to warn about structs also I need to write much more code in the MarkWritten 
etc.



http://reviews.llvm.org/D12359



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

Re: [PATCH] D12359: New warning -Wnonconst-parameter when a pointer parameter can be const

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a comment.

The full output log I got when building clang was very big. 47MB. Roughly half 
of that is lines saying "In file included from". I also saw false positives for 
constructors. The initialisation list is not considered properly.

I reduced the output with this grep command:

grep 'tools/clang/.*\.cpp.*\[-Wnonconst' clang-nonconst.txt > 1.txt

So these are all warnings for cpp files in the clang folder. There are 91 
warnings:

I have not triaged these now.. but plan to do it asap..

/home/danielm/llvm/tools/clang/lib/Basic/IdentifierTable.cpp:577:54: warning: 
parameter 'P' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Basic/SourceLocation.cpp:135:46: warning: 
parameter 'Invalid' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Lex/PPDirectives.cpp:220:40: warning: 
parameter 'ShadowFlag' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Parse/ParseStmtAsm.cpp:111:70: warning: 
parameter 'Context' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Parse/ParseTemplate.cpp:1308:47: warning: 
parameter 'P' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Parse/Parser.cpp:530:54: warning: parameter 
'P' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/AST/Decl.cpp:566:62: warning: parameter 'D' 
can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/AST/Decl.cpp:2101:27: warning: parameter 
'UntypedValue' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/AST/DeclTemplate.cpp:250:51: warning: 
parameter 'Ptr' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/AST/DeclTemplate.cpp:327:48: warning: 
parameter 'Ptr' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/AST/DeclTemplate.cpp:940:52: warning: 
parameter 'Ptr' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/AST/DeclTemplate.cpp:967:46: warning: 
parameter 'Ptr' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/AST/NestedNameSpecifier.cpp:367:43: warning: 
parameter 'Data' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/AST/NestedNameSpecifier.cpp:375:27: warning: 
parameter 'Data' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/AST/NestedNameSpecifier.cpp:469:26: warning: 
parameter 'Ptr' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/AST/StmtPrinter.cpp:705:46: warning: 
parameter 'Node' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/AST/StmtProfile.cpp:337:47: warning: 
parameter 'Node' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Sema/SemaDecl.cpp:2557:47: warning: 
parameter 'D' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Sema/SemaDecl.cpp:2565:50: warning: 
parameter 'New' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Sema/SemaExpr.cpp:13905:55: warning: 
parameter 'E' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Sema/SemaExpr.cpp:14000:55: warning: 
parameter 'E' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:40:72: 
warning: parameter 'NewDecl' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:305:54: 
warning: parameter 'D' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/CodeGen/CodeGenAction.cpp:220:73: warning: 
parameter 'Context' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/CodeGen/CodeGenAction.cpp:229:41: warning: 
parameter 'Context' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Analysis/CFGStmtMap.cpp:23:26: warning: 
parameter 'm' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Analysis/FormatString.cpp:126:57: warning: 
parameter 'argIndex' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Analysis/LiveVariables.cpp:111:41: warning: 
parameter 'x' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Analysis/LiveVariables.cpp:478:36: warning: 
parameter 'im' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Analysis/PrintfFormatString.cpp:39:38: 
warning: parameter 'argIndex' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Frontend/ASTUnit.cpp:2763:45: warning: 
parameter 'context' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Frontend/CompilerInstance.cpp:407:11: 
warning: parameter 'DeserializationListener' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Frontend/CompilerInvocation.cpp:1090:56: 
warning: parameter 'MainAddr' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/clang/lib/Frontend/DependencyFile.cpp:237:56: warning: 
parameter 'Impl' can be const [-Wnonconst-parameter]
/home/danielm/llvm/tools/cla

Re: [PATCH] D12359: New warning -Wnonconst-parameter when a pointer parameter can be const

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a comment.

After looking at 5 of the warnings for Clang.. it is obvious that there are 
lots of FP for this code. I will fix these so a better log for Clang can be 
shown.


http://reviews.llvm.org/D12359



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

RE: r246646 - Silence a -Wsign-compare warning; NFC.

[Daniel Marjamäki via cfe-commits]

Hello!

In general I am scared of such fixes.

I assume we know that Info::MaxTables is never a negative value now. However if 
it would (by mistake) get a negative value in the future then you are hiding an 
important TP warning here.

I assume we know that Info::MaxTables is never larger than int since you casted 
to unsigned. However if it would (by mistake) get a large value and there is 
loss of precision we wont know it because the TP warnings are hidden.

In my opinion, hiding -Wsign-compare FP warnings makes the code weaker.

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.Marjamaki@evidente.se

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

Re: [PATCH] D12619: [Static Analyzer] Minor cleanups for the nullability checker.

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a subscriber: danielmarjamaki.
danielmarjamaki added a comment.

Overall.. LGTM



Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:78
@@ -78,2 +77,3 @@
+  llvm_unreachable("Unexpected enumeration.");
   return "";
 }

After llvm_unreachable there is often not a return.


http://reviews.llvm.org/D12619



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

Re: [PATCH] D12359: New warning -Wnonconst-parameter when a pointer parameter can be const

[Daniel Marjamäki via cfe-commits]

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

Minor cleanups and refactorings


http://reviews.llvm.org/D12359

Files:
  include/clang/AST/Decl.h
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Parse/Parser.h
  include/clang/Sema/Sema.h
  lib/AST/Decl.cpp
  lib/Parse/ParseExpr.cpp
  lib/Parse/ParseStmt.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaLambda.cpp
  lib/Sema/SemaOpenMP.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  lib/Serialization/ASTReaderDecl.cpp
  test/Sema/warn-nonconst-parameter.c
  test/SemaCXX/warn-nonconst-parameter.cpp

Index: test/SemaCXX/warn-nonconst-parameter.cpp
===
--- test/SemaCXX/warn-nonconst-parameter.cpp
+++ test/SemaCXX/warn-nonconst-parameter.cpp
@@ -0,0 +1,44 @@
+// RUN: %clang_cc1 -fsyntax-only -Wnonconst-parameter -verify %s
+//
+// Test that -Wnonconst-parameter does not warn for virtual functions.
+//
+
+// expected-no-diagnostics
+
+class Base {
+public:
+  virtual int f(int*p);
+};
+
+class Derived : public Base {
+public:
+  // Don't warn on inherited virtual function
+  virtual int f(int *p) { return 0; }
+
+  // Don't warn on non-inherited virtual function
+  virtual int g(int *p) { return 0; }
+};
+
+class Fred {
+private:
+  void *X;
+public:
+  Fred(void *X) : X(X) {}
+  void dostuff();
+};
+
+static Fred& dontwarn1(void *P) {
+  return *static_cast(P);
+}
+
+static void dontwarn2(void *P) {
+  ((Fred*)P)->dostuff();
+}
+
+
+// Don't warn when data is passed to nonconst reference parameter
+void dostuff(int &x);
+void dontwarn19(int *p) {
+  dostuff(*p);
+}
+
Index: test/Sema/warn-nonconst-parameter.c
===
--- test/Sema/warn-nonconst-parameter.c
+++ test/Sema/warn-nonconst-parameter.c
@@ -0,0 +1,125 @@
+// RUN: %clang_cc1 -fsyntax-only -Wnonconst-parameter -verify %s
+
+// Currently the -Wnonconst-parameter only warns about pointer arguments.
+//
+// It can be defined both that the data is const and that the pointer is const,
+// the -Wnonconst-parameter 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);
+
+
+int warn1(int *p) { // expected-warning {{parameter 'p' can be const}}
+  return *p;
+}
+
+void warn2(int *first, int *last) { // expected-warning {{parameter 'last' can be const}}
+  *first = 0;
+  if (first < last) {} // <- last can be const
+}
+
+void warn3(char *p) { // expected-warning {{parameter 'p' can be const}}
+  char buf[10];
+  strcpy1(buf, p);
+}
+
+int dontwarn1(const int *p) {
+  return *p;
+}
+
+void dontwarn2(int *p) {
+  *p = 0;
+}
+
+void dontwarn3(char *p) {
+  p[0] = 0;
+}
+
+void dontwarn4(int *p) {
+  int *q = p;
+  *q = 0;
+}
+
+void dontwarn5(float *p) {
+  int *q = (int *)p;
+}
+
+void dontwarn6(char *p) {
+  char *a, *b;
+  a = b = p;
+}
+
+void dontwarn7(int *p) {
+  int *i = p ? p : 0;
+}
+
+void dontwarn8(char *p) {
+  char *x;
+  x = (p ? p : "");
+}
+
+char *dontwarn9(char *p) {
+  char *x;
+  return p ? p : "";
+}
+
+void dontwarn10(int *p) {
+  ++(*p);
+}
+
+char dontwarn11(int *p) {
+  return ++(*p);
+}
+
+char *dontwarn12(char *s) {
+  return s;
+}
+
+void dontwarn13(unsigned char *str, const unsigned int i) {
+unsigned char *p;
+for (p = str + i; *p; ) {}
+}
+
+void dontwarn14(int *buf) {
+  int i, *p;
+  for (i=0, p=buf; i<10; i++, p++) {
+*p = 1;
+  }
+}
+
+void dontwarn15(int *p, int x) {
+for (int *q = p+x-1; 0; q++);
+}
+
+void dontwarn16(char *p) {
+  strcpy1(p, "abc");
+}
+
+void dontwarn17(char *p) {
+  strcpy1(p+2, "abc");
+}
+
+
+char *dontwarn18(char *p) {
+  return strcpy1(p, "abc");
+}
+
+unsigned dontwarn19(char *buf) {
+  unsigned len = my_strlen(buf);
+  return len + my_strcpy(buf, "abc");
+}
+
+// Don't warn about nonconst function pointers that can be const.
+void functionpointer(double f(double), int x) {
+  f(x);
+}
+
+// Don't warn about nonconst record pointers that can be const.
+struct XY { int x; int y; };
+int recordpointer(struct XY *xy) {
+  return xy->x;
+}
Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -501,6 +501,8 @@
   D->setImplicit(Record[Idx++]);
   D->Used = Record[Idx++];
   D->setReferenced(Record[Idx++]);
+  if (auto *VD = dyn_cast(D))
+VD->setNonConstUse();
   D->setTopLevelDeclInObjCContainer(Record[Idx++]);
   D->setAccess((AccessSpecifier)Record[Idx++]);
   D->FromASTFile = true;
Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
=

Re: [PATCH] D12359: New warning -Wnonconst-parameter when a pointer parameter can be const

[Daniel Marjamäki via cfe-commits]

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

I will remove test/SemaCXX/warn-nonconst-parameter.cpp in the next patch since 
this checker does not handle C++ yet.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:201
@@ -200,1 +200,3 @@
+def warn_nonconst_parameter : Warning<"parameter %0 can be const">,
+  InGroup, DefaultIgnore;
 def warn_unused_variable : Warning<"unused variable %0">,

aaron.ballman wrote:
> Off-by-default warnings aren't something we usually want to add to the 
> frontend. They add to the cost of every compile, and are rarely enabled by 
> users. From what I understand (and I could be wrong), we generally only add 
> DefaultIgnore diagnostics if we are emulating GCC behavior.
ok. I personally prefer to only get serious errors by default. I believe people 
should use -Weverything and then -Wno-.. to turn off noise. But I do want to 
follow the Clang best practices here so I fix this.


Comment at: include/clang/Basic/DiagnosticSemaKinds.td:201
@@ -200,1 +200,3 @@
+def warn_nonconst_parameter : Warning<"parameter %0 can be const">,
+  InGroup, DefaultIgnore;
 def warn_unused_variable : Warning<"unused variable %0">,

danielmarjamaki wrote:
> aaron.ballman wrote:
> > Off-by-default warnings aren't something we usually want to add to the 
> > frontend. They add to the cost of every compile, and are rarely enabled by 
> > users. From what I understand (and I could be wrong), we generally only add 
> > DefaultIgnore diagnostics if we are emulating GCC behavior.
> ok. I personally prefer to only get serious errors by default. I believe 
> people should use -Weverything and then -Wno-.. to turn off noise. But I do 
> want to follow the Clang best practices here so I fix this.
> and are rarely enabled by users

I disagree about this. Normal usage is to enable as much warnings as you can.

Is it possible for you to show a document, discussion or something that backs 
your claim? 

When I make it on by default lots of testcases fail. So before fixing all this 
I'd prefer to know that I don't waste my time.



Comment at: lib/Parse/ParseExpr.cpp:176
@@ +175,3 @@
+  if (auto *B = dyn_cast(ER.get())) {
+if (B->isAssignmentOp() || B->isAdditiveOp()) {
+  MarkWritten(B->getLHS());

aaron.ballman wrote:
> I do not understand why addition is included here.
basic idea is that p can't be const here:

void f(int *p) {
int *q = p + 1;
// ...
}


Comment at: lib/Parse/ParseExpr.cpp:181
@@ +180,3 @@
+  } else if (isa(ER.get()) ||
+ isa(ER.get()) ||
+ isa(ER.get())) {

aaron.ballman wrote:
> Or why a conditional expression is included along with unary operators.
This "conditional expression" check ensures that  dontwarn9 does not generate 
FP:

char *dontwarn9(char *p) {
  char *x;
  return p ? p : "";
}

The "unary operator" check ensures that dontwarn11 does not generate FP:

char dontwarn11(int *p) {
  return ++(*p);
}




Comment at: lib/Parse/ParseExprCXX.cpp:2831
@@ -2830,1 +2830,3 @@
 
+  MarkWritten(Operand.get());
+

aaron.ballman wrote:
> Why does this count as a write? Also, if you are not including support for 
> C++ yet, perhaps this should be removed regardless.
ok I remove it. It will be needed later if we want to add support for C++. I've 
tried to add support for C++ but had problems with templates I failed to fix.


Comment at: lib/Parse/ParseStmt.cpp:376
@@ +375,3 @@
+// Mark symbols in r-value expression as written.
+void Parser::MarkWritten(Expr *E) {
+  E = E->IgnoreParenCasts();

This is called from the Parser only.


Comment at: lib/Sema/SemaDecl.cpp:11028
@@ -10972,1 +11027,3 @@
 DiagnoseUnusedParameters(FD->param_begin(), FD->param_end());
+  if (!getLangOpts().CPlusPlus)
+DiagnoseNonConstPointerParameters(FD->param_begin(), FD->param_end());

no. but the c++ check means that is not a problem now.


Comment at: lib/Sema/SemaDecl.cpp:8934
@@ -8933,1 +8933,3 @@
 
+static void MarkWritten(Expr *E) {
+  E = E->IgnoreParenCasts();

yes good points.. figuring out good names are always hard.


Comment at: test/Sema/warn-nonconst-parameter.c:55
@@ +54,3 @@
+}
+
+void dontwarn7(int *p) {

yes I only warn about parameters currently.


http://reviews.llvm.org/D12359



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

Re: [PATCH] D12359: New warning -Wnonconst-parameter when a pointer parameter can be const

[Daniel Marjamäki via cfe-commits]

danielmarjamaki updated this revision to Diff 37582.
danielmarjamaki marked 5 inline comments as done.
danielmarjamaki added a comment.

One more work in progress patch.

I have moved the NonConstUse to VarDecl.

I turned on Wnonconst-parameter by default. This had a very positive effect; I 
saw FPs in some tests that I had to fix.

I have moved the two MarkNonConstUse() together. The names are maybe not ideal 
but having them together means it's easier to compare them now.


http://reviews.llvm.org/D12359

Files:
  include/clang/AST/Decl.h
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/AST/Decl.cpp
  lib/Parse/ParseDecl.cpp
  lib/Parse/ParseExpr.cpp
  lib/Parse/ParseStmt.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaLambda.cpp
  lib/Sema/SemaOpenMP.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  lib/Serialization/ASTReaderDecl.cpp
  test/Analysis/NoReturn.m
  test/Analysis/bstring.c
  test/Analysis/casts.c
  test/Analysis/coverage.c
  test/Analysis/inlining/false-positive-suppression.c
  test/Analysis/inlining/inline-defensive-checks.c
  test/Analysis/inlining/path-notes.m
  test/Analysis/logical-ops.c
  test/Analysis/malloc.c
  test/Analysis/misc-ps-region-store.m
  test/Analysis/misc-ps.c
  test/Analysis/misc-ps.m
  test/Analysis/null-deref-ps.c
  test/Analysis/objc-boxing.m
  test/Analysis/pr22954.c
  test/Analysis/ptr-arith.c
  test/Analysis/retain-release-inline.m
  test/Analysis/retain-release.m
  test/Analysis/simple-stream-checks.c
  test/Analysis/stack-addr-ps.c
  test/Analysis/string.c
  test/Analysis/svalbuilder-logic.c
  test/Analysis/unix-fns.c
  test/CodeGen/builtins-arm-exclusive.c
  test/CodeGen/builtins-systemz.c
  test/Parser/MicrosoftExtensionsInlineAsm.c
  test/Parser/attributes.c
  test/Parser/declarators.c
  test/Parser/pointer-arithmetic.c
  test/Sema/annotate.c
  test/Sema/arm-neon-types.c
  test/Sema/atomic-ops.c
  test/Sema/builtin-assume-aligned.c
  test/Sema/builtin-assume.c
  test/Sema/builtins-arm-exclusive.c
  test/Sema/builtins-arm64-exclusive.c
  test/Sema/builtins.c
  test/Sema/builtins.cl
  test/Sema/c89.c
  test/Sema/compare.c
  test/Sema/crash-invalid-array.c
  test/Sema/empty1.c
  test/Sema/exprs.c
  test/Sema/function.c
  test/Sema/merge-decls.c
  test/Sema/ms-inline-asm.c
  test/Sema/pointer-subtract-compat.c
  test/Sema/transparent-union.c
  test/Sema/typecheck-binop.c
  test/Sema/typo-correction.c
  test/Sema/uninit-variables.c
  test/Sema/unused-expr.c
  test/Sema/varargs-x86-64.c
  test/Sema/warn-logical-not-compare.c
  test/Sema/warn-nonconst-parameter.c
  test/Sema/warn-sizeof-arrayarg.c
  test/Sema/warn-strncat-size.c
  test/Sema/warn-thread-safety-analysis.c
  test/Sema/warn-type-safety-mpi-hdf5.c
  test/SemaObjC/nullability.m
  test/SemaObjC/uninit-variables.m
  test/SemaOpenCL/address-spaces.cl
  test/SemaOpenCL/cond.cl
  test/SemaOpenCL/event_t_overload.cl

Index: test/SemaOpenCL/event_t_overload.cl
===
--- test/SemaOpenCL/event_t_overload.cl
+++ test/SemaOpenCL/event_t_overload.cl
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
+// RUN: %clang_cc1 %s -verify -pedantic -Wno-nonconst-parameter -fsyntax-only
 
 void __attribute__((overloadable)) foo(event_t, __local char *); // expected-note {{candidate function not viable: no known conversion from '__global int *' to '__local char *' for 2nd argument}}
 void __attribute__((overloadable)) foo(event_t, __local float *); // expected-note {{candidate function not viable: no known conversion from '__global int *' to '__local float *' for 2nd argument}}
Index: test/SemaOpenCL/cond.cl
===
--- test/SemaOpenCL/cond.cl
+++ test/SemaOpenCL/cond.cl
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
+// RUN: %clang_cc1 %s -verify -pedantic -Wno-nonconst-parameter -fsyntax-only
 
 typedef unsigned char uchar;
 typedef unsigned char uchar2 __attribute__((ext_vector_type(2)));
Index: test/SemaOpenCL/address-spaces.cl
===
--- test/SemaOpenCL/address-spaces.cl
+++ test/SemaOpenCL/address-spaces.cl
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
+// RUN: %clang_cc1 %s -verify -pedantic -Wno-nonconst-parameter -fsyntax-only
 
 __constant int ci = 1;
 
Index: test/SemaObjC/uninit-variables.m
===
--- test/SemaObjC/uninit-variables.m
+++ test/SemaObjC/uninit-variables.m
@@ -36,7 +36,7 @@
   }
 }
 
-int test_abort_on_exceptions(int y, NSException *e, NSString *s, int *z, ...) {
+int test_abort_on_exceptions(int y, NSException *e, NSString *s, const int *z, ...) {
   int x; // expected-note {{initialize the variable 'x' to silence this warning}}
   if (y == 1) {
 va_list alist;
Index: test/SemaObjC/nullability.m

Re: [PATCH] D12359: New warning -Wnonconst-parameter when a pointer parameter can be const

[Daniel Marjamäki via cfe-commits]

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

Minor fixes from previous patch. Added some more testcases.


http://reviews.llvm.org/D12359

Files:
  include/clang/AST/Decl.h
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/AST/Decl.cpp
  lib/Parse/ParseDecl.cpp
  lib/Parse/ParseExpr.cpp
  lib/Parse/ParseStmt.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaLambda.cpp
  lib/Sema/SemaOpenMP.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  lib/Serialization/ASTReaderDecl.cpp
  test/Analysis/NoReturn.m
  test/Analysis/bstring.c
  test/Analysis/casts.c
  test/Analysis/coverage.c
  test/Analysis/inlining/false-positive-suppression.c
  test/Analysis/inlining/inline-defensive-checks.c
  test/Analysis/inlining/path-notes.m
  test/Analysis/logical-ops.c
  test/Analysis/malloc.c
  test/Analysis/misc-ps-region-store.m
  test/Analysis/misc-ps.c
  test/Analysis/misc-ps.m
  test/Analysis/null-deref-ps.c
  test/Analysis/objc-boxing.m
  test/Analysis/pr22954.c
  test/Analysis/ptr-arith.c
  test/Analysis/retain-release-inline.m
  test/Analysis/retain-release.m
  test/Analysis/simple-stream-checks.c
  test/Analysis/stack-addr-ps.c
  test/Analysis/string.c
  test/Analysis/svalbuilder-logic.c
  test/Analysis/unix-fns.c
  test/CodeGen/builtins-arm-exclusive.c
  test/CodeGen/builtins-systemz.c
  test/FixIt/dereference-addressof.c
  test/Parser/MicrosoftExtensionsInlineAsm.c
  test/Parser/attributes.c
  test/Parser/declarators.c
  test/Parser/pointer-arithmetic.c
  test/Sema/annotate.c
  test/Sema/arm-neon-types.c
  test/Sema/atomic-ops.c
  test/Sema/builtin-assume-aligned.c
  test/Sema/builtin-assume.c
  test/Sema/builtins-arm-exclusive.c
  test/Sema/builtins-arm64-exclusive.c
  test/Sema/builtins.c
  test/Sema/builtins.cl
  test/Sema/c89.c
  test/Sema/compare.c
  test/Sema/crash-invalid-array.c
  test/Sema/empty1.c
  test/Sema/exprs.c
  test/Sema/function.c
  test/Sema/merge-decls.c
  test/Sema/ms-inline-asm.c
  test/Sema/pointer-subtract-compat.c
  test/Sema/transparent-union.c
  test/Sema/typecheck-binop.c
  test/Sema/typo-correction.c
  test/Sema/uninit-variables.c
  test/Sema/unused-expr.c
  test/Sema/varargs-x86-64.c
  test/Sema/warn-logical-not-compare.c
  test/Sema/warn-nonconst-parameter.c
  test/Sema/warn-sizeof-arrayarg.c
  test/Sema/warn-strncat-size.c
  test/Sema/warn-thread-safety-analysis.c
  test/Sema/warn-type-safety-mpi-hdf5.c
  test/SemaObjC/nullability.m
  test/SemaObjC/uninit-variables.m
  test/SemaOpenCL/address-spaces.cl
  test/SemaOpenCL/cond.cl
  test/SemaOpenCL/event_t_overload.cl

Index: test/SemaOpenCL/event_t_overload.cl
===
--- test/SemaOpenCL/event_t_overload.cl
+++ test/SemaOpenCL/event_t_overload.cl
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
+// RUN: %clang_cc1 %s -verify -pedantic -Wno-nonconst-parameter -fsyntax-only
 
 void __attribute__((overloadable)) foo(event_t, __local char *); // expected-note {{candidate function not viable: no known conversion from '__global int *' to '__local char *' for 2nd argument}}
 void __attribute__((overloadable)) foo(event_t, __local float *); // expected-note {{candidate function not viable: no known conversion from '__global int *' to '__local float *' for 2nd argument}}
Index: test/SemaOpenCL/cond.cl
===
--- test/SemaOpenCL/cond.cl
+++ test/SemaOpenCL/cond.cl
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
+// RUN: %clang_cc1 %s -verify -pedantic -Wno-nonconst-parameter -fsyntax-only
 
 typedef unsigned char uchar;
 typedef unsigned char uchar2 __attribute__((ext_vector_type(2)));
Index: test/SemaOpenCL/address-spaces.cl
===
--- test/SemaOpenCL/address-spaces.cl
+++ test/SemaOpenCL/address-spaces.cl
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
+// RUN: %clang_cc1 %s -verify -pedantic -Wno-nonconst-parameter -fsyntax-only
 
 __constant int ci = 1;
 
Index: test/SemaObjC/uninit-variables.m
===
--- test/SemaObjC/uninit-variables.m
+++ test/SemaObjC/uninit-variables.m
@@ -36,7 +36,7 @@
   }
 }
 
-int test_abort_on_exceptions(int y, NSException *e, NSString *s, int *z, ...) {
+int test_abort_on_exceptions(int y, NSException *e, NSString *s, const int *z, ...) {
   int x; // expected-note {{initialize the variable 'x' to silence this warning}}
   if (y == 1) {
 va_list alist;
Index: test/SemaObjC/nullability.m
===
--- test/SemaObjC/nullability.m
+++ test/SemaObjC/nullability.m
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -fblocks -Woverriding-method-mismatch -Wno-nullability-declspec %s 

Re: [PATCH] D12359: New warning -Wnonconst-parameter when a pointer parameter can be const

[Daniel Marjamäki via cfe-commits]

danielmarjamaki updated this revision to Diff 37728.
danielmarjamaki marked an inline comment as done.
danielmarjamaki added a comment.

Fix FN for code:

const char *ret(char *p) {

  return p ? p : "";

}


http://reviews.llvm.org/D12359

Files:
  include/clang/AST/Decl.h
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/AST/Decl.cpp
  lib/Parse/ParseDecl.cpp
  lib/Parse/ParseExpr.cpp
  lib/Parse/ParseStmt.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaLambda.cpp
  lib/Sema/SemaOpenMP.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  lib/Serialization/ASTReaderDecl.cpp
  test/Analysis/NoReturn.m
  test/Analysis/bstring.c
  test/Analysis/casts.c
  test/Analysis/coverage.c
  test/Analysis/inlining/false-positive-suppression.c
  test/Analysis/inlining/inline-defensive-checks.c
  test/Analysis/inlining/path-notes.m
  test/Analysis/logical-ops.c
  test/Analysis/malloc.c
  test/Analysis/misc-ps-region-store.m
  test/Analysis/misc-ps.c
  test/Analysis/misc-ps.m
  test/Analysis/null-deref-ps.c
  test/Analysis/objc-boxing.m
  test/Analysis/pr22954.c
  test/Analysis/ptr-arith.c
  test/Analysis/retain-release-inline.m
  test/Analysis/retain-release.m
  test/Analysis/simple-stream-checks.c
  test/Analysis/stack-addr-ps.c
  test/Analysis/string.c
  test/Analysis/svalbuilder-logic.c
  test/Analysis/unix-fns.c
  test/CodeGen/builtins-arm-exclusive.c
  test/CodeGen/builtins-systemz.c
  test/FixIt/dereference-addressof.c
  test/Parser/MicrosoftExtensionsInlineAsm.c
  test/Parser/attributes.c
  test/Parser/declarators.c
  test/Parser/pointer-arithmetic.c
  test/Sema/annotate.c
  test/Sema/arm-neon-types.c
  test/Sema/atomic-ops.c
  test/Sema/builtin-assume-aligned.c
  test/Sema/builtin-assume.c
  test/Sema/builtins-arm-exclusive.c
  test/Sema/builtins-arm64-exclusive.c
  test/Sema/builtins.c
  test/Sema/builtins.cl
  test/Sema/c89.c
  test/Sema/compare.c
  test/Sema/crash-invalid-array.c
  test/Sema/empty1.c
  test/Sema/exprs.c
  test/Sema/function.c
  test/Sema/merge-decls.c
  test/Sema/ms-inline-asm.c
  test/Sema/pointer-subtract-compat.c
  test/Sema/transparent-union.c
  test/Sema/typecheck-binop.c
  test/Sema/typo-correction.c
  test/Sema/uninit-variables.c
  test/Sema/unused-expr.c
  test/Sema/varargs-x86-64.c
  test/Sema/warn-logical-not-compare.c
  test/Sema/warn-nonconst-parameter.c
  test/Sema/warn-sizeof-arrayarg.c
  test/Sema/warn-strncat-size.c
  test/Sema/warn-thread-safety-analysis.c
  test/Sema/warn-type-safety-mpi-hdf5.c
  test/SemaObjC/nullability.m
  test/SemaObjC/uninit-variables.m
  test/SemaOpenCL/address-spaces.cl
  test/SemaOpenCL/cond.cl
  test/SemaOpenCL/event_t_overload.cl

Index: test/SemaOpenCL/event_t_overload.cl
===
--- test/SemaOpenCL/event_t_overload.cl
+++ test/SemaOpenCL/event_t_overload.cl
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
+// RUN: %clang_cc1 %s -verify -pedantic -Wno-nonconst-parameter -fsyntax-only
 
 void __attribute__((overloadable)) foo(event_t, __local char *); // expected-note {{candidate function not viable: no known conversion from '__global int *' to '__local char *' for 2nd argument}}
 void __attribute__((overloadable)) foo(event_t, __local float *); // expected-note {{candidate function not viable: no known conversion from '__global int *' to '__local float *' for 2nd argument}}
Index: test/SemaOpenCL/cond.cl
===
--- test/SemaOpenCL/cond.cl
+++ test/SemaOpenCL/cond.cl
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
+// RUN: %clang_cc1 %s -verify -pedantic -Wno-nonconst-parameter -fsyntax-only
 
 typedef unsigned char uchar;
 typedef unsigned char uchar2 __attribute__((ext_vector_type(2)));
Index: test/SemaOpenCL/address-spaces.cl
===
--- test/SemaOpenCL/address-spaces.cl
+++ test/SemaOpenCL/address-spaces.cl
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
+// RUN: %clang_cc1 %s -verify -pedantic -Wno-nonconst-parameter -fsyntax-only
 
 __constant int ci = 1;
 
Index: test/SemaObjC/uninit-variables.m
===
--- test/SemaObjC/uninit-variables.m
+++ test/SemaObjC/uninit-variables.m
@@ -36,7 +36,7 @@
   }
 }
 
-int test_abort_on_exceptions(int y, NSException *e, NSString *s, int *z, ...) {
+int test_abort_on_exceptions(int y, NSException *e, NSString *s, const int *z, ...) {
   int x; // expected-note {{initialize the variable 'x' to silence this warning}}
   if (y == 1) {
 va_list alist;
Index: test/SemaObjC/nullability.m
===
--- test/SemaObjC/nullability.m
+++ test/SemaObjC/nullability.m
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -fblocks -Woverriding-method-mismatch -Wno-nullability-de

Re: [PATCH] D12359: New warning -Wnonconst-parameter when a pointer parameter can be const

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a comment.

In http://reviews.llvm.org/D12359#233152, @sberg wrote:

> causes false positive for
>
>   char * f(char *);
>   char * g(char * p) { return f(p); }
>


Sorry for replying this late. This should work in latest patch.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:201
@@ -200,1 +200,3 @@
+def warn_nonconst_parameter : Warning<"parameter %0 can be const">,
+  InGroup, DefaultIgnore;
 def warn_unused_variable : Warning<"unused variable %0">,

aaron.ballman wrote:
> > I disagree about this. Normal usage is to enable as much warnings as you 
> > can.
> > 
> > Is it possible for you to show a document, discussion or something that 
> > backs your claim?
> 
> Searching through the Clang email archives yields:
> 
> http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20150504/128373.html
> http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20140922/115379.html
> 
> and others as well. This has been the de facto bar for as long as I've been 
> contributing.
ok thanks for looking it up. I will try to fix all the test cases.


Comment at: lib/Parse/ParseExpr.cpp:176
@@ +175,3 @@
+  if (auto *B = dyn_cast(ER.get())) {
+if (B->isAssignmentOp() || B->isAdditiveOp()) {
+  MarkNonConstUse(B->getLHS());

aaron.ballman wrote:
> > basic idea is that p can't be const here:
> ```
> void f(int *p) {
> int *q = p + 1;
> // ...
> }
> ```
> But it could be const here:
> ```
> void f(int *p) {
>   const *q = p + 1;
> }
> ```
> I am not certain that addition, by itself, is sufficient to say the use is 
> non-const. At the least, this could have some comments explaining the 
> rationale with a FIXME.
that is not by intention. There is no nonconst use if lhs is a const pointer. I 
will investigate.


Comment at: lib/Parse/ParseExpr.cpp:176
@@ +175,3 @@
+  Expr *E = ER.get()->IgnoreParenCasts();
+  if (auto *B = dyn_cast(E)) {
+if (B->isAssignmentOp()) {

it's handled better now.


Comment at: lib/Parse/ParseExpr.cpp:181
@@ +180,3 @@
+ isa(E) ||
+ isa(E) ||
+ isa(E)) {

yes.

dontwarn9 was just a test.. if a nonconst pointer is returned then the 
parameter can't be const. I have corrected the test, see return6 .

I will add a new test in the next iteration when a const pointer is returned.


Comment at: lib/Parse/ParseStmt.cpp:376
@@ +375,3 @@
+// Mark symbols in r-value expression as written.
+void Parser::MarkNonConstUse(Expr *E) {
+  E = E->IgnoreParenCasts();

aaron.ballman wrote:
> > This is called from the Parser only.
> 
> So will this still properly diagnose the same cases from a serialized AST?
I don't know what this "serialized AST" is that you are talking about. All I 
know is the -ast-dump and that flag is only intended as a debugging aid as far 
as I know. If I just run "clang -cc1 -Wnonconst-parameter somefile.c" then it 
does not serialize does it? So what flags do I use to serialize etc?

I would appreciate if you can show me a command that will cause FP. So I can 
look at it.

I believe that we can't put this in the Sema only. The parser knows better how 
the result is used and can set "nonconstuse" properly for expressions.

For instance I don't want to mark all pointer additions as "nonconstuse" just 
because some of them are "nonconstuse".



http://reviews.llvm.org/D12359



___
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.

> It might be more useful if you could print the paths on which the errors 
> occurred (this could be done for text output with -analyzer-output=text)


Sounds good. Is it possible to use it with scan-build?



Comment at: lib/StaticAnalyzer/Checkers/CMakeLists.txt:29
@@ -28,2 +28,3 @@
   ChrootChecker.cpp
+  ConversionChecker.cpp
   ClangCheckers.cpp

hmm.. I will move this down 1 line


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#270193, @danielmarjamaki wrote:

> > It might be more useful if you could print the paths on which the errors 
> > occurred (this could be done for text output with -analyzer-output=text)
>
>
> Sounds good. Is it possible to use it with scan-build?


Sorry. I saw that this has been discussed recently on cfe-dev.

http://lists.llvm.org/pipermail/cfe-dev/2015-October/045292.html


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] D13643: [Sema] Warn on ternary comparison

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a subscriber: danielmarjamaki.
danielmarjamaki added a comment.

Interesting checker.

I'll test it on some debian projects.

If you're interested.. it does not currently warn about "1.0 < 2.0 < 3.0" as 
far as I see.


http://reviews.llvm.org/D13643



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

Re: [PATCH] D13643: [Sema] Warn on ternary comparison

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a comment.

I tested this on 2199 debian projects.. and got 16 warnings.

  ftp://ftp.se.debian.org/debian/pool/main/t/tf5/tf5_5.0beta8.orig.tar.gz
  expr.c:738:22: warning: comparisons such as 'a < b != c' do not have their 
mathematical meaning [-Wparentheses]
  if (neg0 == neg1 && sum<0 != neg0) {
  ^

I guess it could be intentional. I haven't looked in the real code.

  ftp://ftp.se.debian.org/debian/pool/main/m/medusa/medusa_2.1.1.orig.tar.gz
  cvs.c:111:10: warning: comparisons such as 'a <= b <= c' do not have their 
mathematical meaning [-Wparentheses]
if ( !(0 <= argc <= 3) )
   ^~
  ftp.c:140:10: warning: comparisons such as 'a <= b <= c' do not have their 
mathematical meaning [-Wparentheses]
if ( !(0 <= argc <= 3) )
   ^~
  imap.c:143:10: warning: comparisons such as 'a <= b <= c' do not have their 
mathematical meaning [-Wparentheses]
if ( !(0 <= argc <= 2) )
   ^~
  mysql.c:135:10: warning: comparisons such as 'a <= b <= c' do not have their 
mathematical meaning [-Wparentheses]
if ( !(0 <= argc <= 3) )
   ^~
  nntp.c:110:10: warning: comparisons such as 'a <= b <= c' do not have their 
mathematical meaning [-Wparentheses]
if ( !(0 <= argc <= 3) )
   ^~
  pcanywhere.c:133:10: warning: comparisons such as 'a <= b <= c' do not have 
their mathematical meaning [-Wparentheses]
if ( !(0 <= argc <= 3) )
   ^~
  pop3.c:133:10: warning: comparisons such as 'a <= b <= c' do not have their 
mathematical meaning [-Wparentheses]
if ( !(0 <= argc <= 3) )
   ^~
  rexec.c:95:10: warning: comparisons such as 'a <= b <= c' do not have their 
mathematical meaning [-Wparentheses]
if ( !(0 <= argc <= 3) )
   ^~
  rlogin.c:94:10: warning: comparisons such as 'a <= b <= c' do not have their 
mathematical meaning [-Wparentheses]
if ( !(0 <= argc <= 3) )
   ^~
  rsh.c:96:10: warning: comparisons such as 'a <= b <= c' do not have their 
mathematical meaning [-Wparentheses]
if ( !(0 <= argc <= 3) )
   ^~
  smtp.c:135:10: warning: comparisons such as 'a <= b <= c' do not have their 
mathematical meaning [-Wparentheses]
if ( !(0 <= argc <= 3) )
   ^~
  smtp-vrfy.c:114:10: warning: comparisons such as 'a <= b <= c' do not have 
their mathematical meaning [-Wparentheses]
if ( !(0 <= argc <= 3) )
   ^~
  snmp.c:154:10: warning: comparisons such as 'a <= b <= c' do not have their 
mathematical meaning [-Wparentheses]
if ( !(0 <= argc <= 3) )
   ^~
  telnet.c:143:10: warning: comparisons such as 'a <= b <= c' do not have their 
mathematical meaning [-Wparentheses]
if ( !(0 <= argc <= 3) )
   ^~
  vmauthd.c:109:10: warning: comparisons such as 'a <= b <= c' do not have 
their mathematical meaning [-Wparentheses]
if ( !(0 <= argc <= 3) )
   ^~

these are surely true positives.


http://reviews.llvm.org/D13643



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

Re: [PATCH] D12359: New warning -Wnonconst-parameter when a pointer parameter can be const

[Daniel Marjamäki via cfe-commits]

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

Fixed a FP.

Triaged warnings (random selection of warnings):

ftp://ftp.se.debian.org/debian/pool/main/a/aespipe/aespipe_2.4c.orig.tar.bz2
./aespipe.c:467:43: warning: parameter 'keyStr' can be const 
[-Wnonconst-parameter]
 => TP
./rmd160.c:188:38: warning: parameter 'data' can be const [-Wnonconst-parameter]
 => TP

ftp://ftp.se.debian.org/debian/pool/main/b/barcode/barcode_0.98+debian.orig.tar.gz
library.c:37:43: warning: parameter 'text' can be const [-Wnonconst-parameter]
 => TP
ean.c:101:36: warning: parameter 'text' can be const [-Wnonconst-parameter]
 => TP
ean.c:134:39: warning: parameter 'text' can be const [-Wnonconst-parameter]
 => TP
ean.c:178:41: warning: parameter 'text' can be const [-Wnonconst-parameter]
 => TP
ean.c:332:39: warning: parameter 'text' can be const [-Wnonconst-parameter]
 => TP
ean.c:379:40: warning: parameter 'text' can be const [-Wnonconst-parameter]
 => TP
code39.c:64:38: warning: parameter 'text' can be const [-Wnonconst-parameter]
 => TP
code93.c:68:38: warning: parameter 'text' can be const [-Wnonconst-parameter]
 => TP
msi.c:41:39: warning: parameter 'text' can be const [-Wnonconst-parameter]
 => TP
plessey.c:44:39: warning: parameter 'text' can be const [-Wnonconst-parameter]
 => TP
codabar.c:53:39: warning: parameter 'text' can be const [-Wnonconst-parameter]
 => TP

ftp://ftp.se.debian.org/debian/pool/main/c/c-ares/c-ares_1.9.1.orig.tar.gz
ares_gethostbyaddr.c:144:42: warning: parameter 'abuf' can be const 
[-Wnonconst-parameter]
 => TP
ares_gethostbyname.c:180:42: warning: parameter 'abuf' can be const 
[-Wnonconst-parameter]
 => TP

ftp://ftp.se.debian.org/debian/pool/main/c/ccache/ccache_3.2.4.orig.tar.bz2
mdfour.c:41:20: warning: parameter 'M' can be const [-Wnonconst-parameter]
 => TP

ftp://ftp.se.debian.org/debian/pool/main/d/dmalloc/dmalloc_5.5.2.orig.tar.gz
dmalloc_tab.c:297:64: warning: parameter 'last_p' can be const 
[-Wnonconst-parameter]
 => TP

ncpfs:
./nwcrypt.c:306:33: warning: parameter 'new' can be const [-Wnonconst-parameter]
 => TP
ncplib.c:313:38: warning: parameter 'node' can be const [-Wnonconst-parameter]
 => TP
ncplib.c:431:41: warning: parameter 'addrlen' can be const 
[-Wnonconst-parameter]
 => FP (fail to reproduce in smaller example, there is compiler error that 
maybe cause wrong warnings)
ncplib.c:2412:23: warning: parameter 'argc' can be const [-Wnonconst-parameter]
 => FP (fail to reproduce in smaller example, there is compiler error that 
maybe cause wrong warnings)

ftp://ftp.se.debian.org/debian/pool/main/r/readline5/readline5_5.2.orig.tar.gz
vi_mode.c:1429:12: warning: parameter 'mb' can be const [-Wnonconst-parameter]
 => TP
parens.c:150:12: warning: parameter 'string' can be const [-Wnonconst-parameter]
 => TP
search.c:115:12: warning: parameter 'string' can be const [-Wnonconst-parameter]
 => TP
complete.c:479:12: warning: parameter 'filename' can be const 
[-Wnonconst-parameter]
 => TP
complete.c:784:12: warning: parameter 's' can be const [-Wnonconst-parameter]
 => TP
complete.c:1514:12: warning: parameter 'text' can be const 
[-Wnonconst-parameter]
 => TP
bind.c:1076:12: warning: parameter 'args' can be const [-Wnonconst-parameter]
 => Technically TP (char *e = strchr(args, '\n'))
bind.c:1725:12: warning: parameter 'name' can be const [-Wnonconst-parameter]
 => TP
bind.c:2301:12: warning: parameter 'string' can be const [-Wnonconst-parameter]
 => TP
isearch.c:156:12: warning: parameter 'search_string' can be const 
[-Wnonconst-parameter]
 => TP
display.c:2139:12: warning: parameter 'string' can be const 
[-Wnonconst-parameter]
 => TP
histexpand.c:310:12: warning: parameter 'string' can be const 
[-Wnonconst-parameter]
 => TP
histexpand.c:365:13: warning: parameter 's' can be const [-Wnonconst-parameter]
 => TP
histexpand.c:1239:12: warning: parameter 'spec' can be const 
[-Wnonconst-parameter]
 => TP
histexpand.c:1239:19: warning: parameter 'from' can be const 
[-Wnonconst-parameter]
 => TP
histexpand.c:1577:12: warning: parameter 'line' can be const 
[-Wnonconst-parameter]
 => TP
mbutil.c:145:12: warning: parameter 'string' can be const [-Wnonconst-parameter]
 => TP
mbutil.c:205:12: warning: parameter 'src' can be const [-Wnonconst-parameter]
 => TP
mbutil.c:265:12: warning: parameter 'string' can be const [-Wnonconst-parameter]
 => TP
mbutil.c:304:12: warning: parameter 'string' can be const [-Wnonconst-parameter]
 => TP
mbutil.c:306:12: warning: parameter 'mbchar' can be const [-Wnonconst-parameter]
 => TP
mbutil.c:322:12: warning: parameter 'buf' can be const [-Wnonconst-parameter]
 => TP
 ../tilde.c:323:12: warning: parameter 'prefix' can be const 
[-Wnonconst-parameter]
 => TP

ftp://ftp.se.debian.org/debian/pool/main/t/tig/tig_2.0.3.orig.tar.gz
src/tig.c:414:24: warning: parameter 'name' can be const [-Wnonconst-parameter]
 => TP
src/io.c:415:37: warning: parameter 'data' can be const [-Wnonconst-parameter]
 => TP
src/graph.c:13

Re: [PATCH] D13643: [Sema] Warn on ternary comparison

[Daniel Marjamäki via cfe-commits]

danielmarjamaki added a comment.

In http://reviews.llvm.org/D13643#271885, @danielmarjamaki wrote:

> I tested this on 2199 debian projects.. and got 16 warnings.
>
>   ftp://ftp.se.debian.org/debian/pool/main/t/tf5/tf5_5.0beta8.orig.tar.gz
>   expr.c:738:22: warning: comparisons such as 'a < b != c' do not have their 
> mathematical meaning [-Wparentheses]
>   if (neg0 == neg1 && sum<0 != neg0) {
>   ^
>   
>
> I guess it could be intentional. I haven't looked in the real code.
>  


Do you have any comments on that one.


http://reviews.llvm.org/D13643



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

Re: [PATCH] D12359: New warning -Wnonconst-parameter when a pointer parameter can be const

[Daniel Marjamäki via cfe-commits]

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

fixes of review comments


http://reviews.llvm.org/D12359

Files:
  include/clang/AST/Decl.h
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/AST/Decl.cpp
  lib/Parse/ParseDecl.cpp
  lib/Parse/ParseExpr.cpp
  lib/Parse/ParseStmt.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaLambda.cpp
  lib/Sema/SemaOpenMP.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  lib/Serialization/ASTReaderDecl.cpp
  test/Analysis/NoReturn.m
  test/Analysis/bstring.c
  test/Analysis/casts.c
  test/Analysis/coverage.c
  test/Analysis/inlining/false-positive-suppression.c
  test/Analysis/inlining/inline-defensive-checks.c
  test/Analysis/inlining/path-notes.m
  test/Analysis/logical-ops.c
  test/Analysis/malloc.c
  test/Analysis/misc-ps-region-store.m
  test/Analysis/misc-ps.c
  test/Analysis/misc-ps.m
  test/Analysis/null-deref-ps.c
  test/Analysis/objc-boxing.m
  test/Analysis/pr22954.c
  test/Analysis/ptr-arith.c
  test/Analysis/retain-release-inline.m
  test/Analysis/retain-release.m
  test/Analysis/simple-stream-checks.c
  test/Analysis/stack-addr-ps.c
  test/Analysis/string.c
  test/Analysis/svalbuilder-logic.c
  test/Analysis/unix-fns.c
  test/CodeGen/builtins-arm-exclusive.c
  test/CodeGen/builtins-systemz.c
  test/FixIt/dereference-addressof.c
  test/Parser/MicrosoftExtensionsInlineAsm.c
  test/Parser/attributes.c
  test/Parser/declarators.c
  test/Parser/pointer-arithmetic.c
  test/Sema/annotate.c
  test/Sema/arm-neon-types.c
  test/Sema/atomic-ops.c
  test/Sema/builtin-assume-aligned.c
  test/Sema/builtin-assume.c
  test/Sema/builtins-arm-exclusive.c
  test/Sema/builtins-arm64-exclusive.c
  test/Sema/builtins.c
  test/Sema/builtins.cl
  test/Sema/c89.c
  test/Sema/compare.c
  test/Sema/crash-invalid-array.c
  test/Sema/empty1.c
  test/Sema/exprs.c
  test/Sema/function.c
  test/Sema/merge-decls.c
  test/Sema/ms-inline-asm.c
  test/Sema/pointer-subtract-compat.c
  test/Sema/transparent-union.c
  test/Sema/typecheck-binop.c
  test/Sema/typo-correction.c
  test/Sema/uninit-variables.c
  test/Sema/unused-expr.c
  test/Sema/varargs-x86-64.c
  test/Sema/warn-logical-not-compare.c
  test/Sema/warn-nonconst-parameter.c
  test/Sema/warn-sizeof-arrayarg.c
  test/Sema/warn-strncat-size.c
  test/Sema/warn-thread-safety-analysis.c
  test/Sema/warn-type-safety-mpi-hdf5.c
  test/SemaObjC/nullability.m
  test/SemaObjC/uninit-variables.m
  test/SemaOpenCL/address-spaces.cl
  test/SemaOpenCL/cond.cl
  test/SemaOpenCL/event_t_overload.cl

Index: test/SemaOpenCL/event_t_overload.cl
===
--- test/SemaOpenCL/event_t_overload.cl
+++ test/SemaOpenCL/event_t_overload.cl
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
+// RUN: %clang_cc1 %s -verify -pedantic -Wno-nonconst-parameter -fsyntax-only
 
 void __attribute__((overloadable)) foo(event_t, __local char *); // expected-note {{candidate function not viable: no known conversion from '__global int *' to '__local char *' for 2nd argument}}
 void __attribute__((overloadable)) foo(event_t, __local float *); // expected-note {{candidate function not viable: no known conversion from '__global int *' to '__local float *' for 2nd argument}}
Index: test/SemaOpenCL/cond.cl
===
--- test/SemaOpenCL/cond.cl
+++ test/SemaOpenCL/cond.cl
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
+// RUN: %clang_cc1 %s -verify -pedantic -Wno-nonconst-parameter -fsyntax-only
 
 typedef unsigned char uchar;
 typedef unsigned char uchar2 __attribute__((ext_vector_type(2)));
Index: test/SemaOpenCL/address-spaces.cl
===
--- test/SemaOpenCL/address-spaces.cl
+++ test/SemaOpenCL/address-spaces.cl
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
+// RUN: %clang_cc1 %s -verify -pedantic -Wno-nonconst-parameter -fsyntax-only
 
 __constant int ci = 1;
 
Index: test/SemaObjC/uninit-variables.m
===
--- test/SemaObjC/uninit-variables.m
+++ test/SemaObjC/uninit-variables.m
@@ -36,7 +36,7 @@
   }
 }
 
-int test_abort_on_exceptions(int y, NSException *e, NSString *s, int *z, ...) {
+int test_abort_on_exceptions(int y, NSException *e, NSString *s, const int *z, ...) {
   int x; // expected-note {{initialize the variable 'x' to silence this warning}}
   if (y == 1) {
 va_list alist;
Index: test/SemaObjC/nullability.m
===
--- test/SemaObjC/nullability.m
+++ test/SemaObjC/nullability.m
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -fblocks -Woverriding-method-mismatch -Wno-nullability-declspec %s -verify
+// RUN: %clang_cc1 -fsynta