[PATCH] D19586: Misleading Indentation check

2017-02-14 Thread Gábor Horváth via Phabricator via cfe-commits 
xazax.hun added inline comments.



Comment at: clang-tidy/readability/MisleadingIndentationCheck.cpp:79
+  Finder->addMatcher(
+  compoundStmt(anyOf(has(ifStmt()), has(forStmt()), has(whileStmt(
+  .bind("compound"),

alexfh wrote:
> xazax.hun wrote:
> > alexfh wrote:
> > > `has(anyOf(ifStmt(), forStmt(), whileStmt()))` would read better.
> > I agree but unfortunately that does not compile. 
> Yep, with matchers you never know what will compile ;)
> 
> Maybe `has(stmt(anyOf(ifStmt(), forStmt(), whileStmt(`? If this also 
> doesn't work, then it's fine as is.
That did the trick, thanks :)


Repository:
  rL LLVM

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] D19586: Misleading Indentation check

2017-02-14 Thread Phabricator via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rL295041: [clang-tidy] Add readability-misleading-indentation 
check. (authored by xazax).

Changed prior to commit:
  https://reviews.llvm.org/D19586?vs=88330=88334#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D19586

Files:
  clang-tools-extra/trunk/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/trunk/clang-tidy/readability/MisleadingIndentationCheck.cpp
  clang-tools-extra/trunk/clang-tidy/readability/MisleadingIndentationCheck.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-misleading-indentation.rst
  clang-tools-extra/trunk/test/clang-tidy/readability-misleading-indentation.cpp

Index: clang-tools-extra/trunk/test/clang-tidy/readability-misleading-indentation.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/readability-misleading-indentation.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/readability-misleading-indentation.cpp
@@ -0,0 +1,80 @@
+// RUN: %check_clang_tidy %s readability-misleading-indentation %t
+
+void foo1();
+void foo2();
+
+#define BLOCK \
+  if (cond1)  \
+foo1();   \
+foo2();
+
+int main()
+{
+  bool cond1 = true;
+  bool cond2 = true;
+
+  if (cond1)
+if (cond2)
+  foo1();
+  else
+foo2();
+  // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: different indentation for 'if' and corresponding 'else' [readability-misleading-indentation]
+
+  if (cond1) {
+if (cond2)
+  foo1();
+  }
+  else
+foo2();
+
+  if (cond1)
+if (cond2)
+  foo1();
+else
+  foo2();
+
+  if (cond2)
+foo1();
+foo2();
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: misleading indentation: statement is indented too deeply [readability-misleading-indentation]
+// CHECK-MESSAGES: :[[@LINE-4]]:3: note: did you mean this line to be inside this 'if'
+foo2(); // No redundant warning.
+
+  if (cond1)
+  {
+foo1();
+  }
+foo2();
+
+  if (cond1)
+foo1();
+  foo2();
+
+  if (cond2)
+if (cond1) foo1(); else foo2();
+
+  if (cond1) {
+  } else {
+  }
+
+  if (cond1) {
+  }
+  else {
+  }
+
+  if (cond1)
+  {
+  }
+  else
+  {
+  }
+
+  if (cond1)
+{
+}
+  else
+{
+}
+
+  BLOCK
+}
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
@@ -11,6 +11,7 @@
   IdentifierNamingCheck.cpp
   ImplicitBoolCastCheck.cpp
   InconsistentDeclarationParameterNameCheck.cpp
+  MisleadingIndentationCheck.cpp
   MisplacedArrayIndexCheck.cpp
   NamedParameterCheck.cpp
   NamespaceCommentCheck.cpp
Index: clang-tools-extra/trunk/clang-tidy/readability/MisleadingIndentationCheck.h
===
--- clang-tools-extra/trunk/clang-tidy/readability/MisleadingIndentationCheck.h
+++ clang-tools-extra/trunk/clang-tidy/readability/MisleadingIndentationCheck.h
@@ -0,0 +1,41 @@
+//===--- MisleadingIndentationCheck.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_MISLEADING_INDENTATION_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_MISLEADING_INDENTATION_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Checks the code for dangling else, and possible misleading indentations due
+/// to missing braces. Note that this check only works as expected when the tabs
+/// or spaces are used consistently and not mixed.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-misleading-indentation.html
+class MisleadingIndentationCheck : public ClangTidyCheck {
+public:
+  MisleadingIndentationCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult ) override;
+
+private:
+  void danglingElseCheck(const SourceManager , const IfStmt *If);
+  void missingBracesCheck(const SourceManager , const CompoundStmt *CStmt);
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_MISLEADING_INDENTATION_H
Index: clang-tools-extra/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp

[PATCH] D19586: Misleading Indentation check

2017-02-14 Thread Alexander Kornienko via Phabricator via cfe-commits 
alexfh added a comment.

Gábor, thank you for picking up this patch and finishing it!


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] D19586: Misleading Indentation check

2017-02-14 Thread Alexander Kornienko via Phabricator via cfe-commits 
alexfh accepted this revision.
alexfh added a comment.
This revision is now accepted and ready to land.

LG




Comment at: clang-tidy/readability/MisleadingIndentationCheck.cpp:79
+  Finder->addMatcher(
+  compoundStmt(anyOf(has(ifStmt()), has(forStmt()), has(whileStmt(
+  .bind("compound"),

xazax.hun wrote:
> alexfh wrote:
> > `has(anyOf(ifStmt(), forStmt(), whileStmt()))` would read better.
> I agree but unfortunately that does not compile. 
Yep, with matchers you never know what will compile ;)

Maybe `has(stmt(anyOf(ifStmt(), forStmt(), whileStmt(`? If this also 
doesn't work, then it's fine as is.


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] D19586: Misleading Indentation check

2017-02-14 Thread Gábor Horváth via Phabricator via cfe-commits 
xazax.hun added a comment.

Thank you for the review!




Comment at: clang-tidy/readability/MisleadingIndentationCheck.cpp:79
+  Finder->addMatcher(
+  compoundStmt(anyOf(has(ifStmt()), has(forStmt()), has(whileStmt(
+  .bind("compound"),

alexfh wrote:
> `has(anyOf(ifStmt(), forStmt(), whileStmt()))` would read better.
I agree but unfortunately that does not compile. 



Comment at: test/clang-tidy/readability-misleading-indentation.cpp:21
+foo2();
+  // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: potential dangling 'else' 
[readability-misleading-indentation]
+

danielmarjamaki wrote:
> I am skeptic about this warning message.
> 
> Why does it say "potential". I would say that in this test case the 
> indentation _is_ "dangling".
> 
> The message is not very clear to me. I personally don't intuitively 
> understand what is wrong without looking at the code.
> 
> I don't know what it should say. Maybe:
> ```
> different indentation for 'if' and 'else'
> ```
> 
Thank you, good idea!


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] D19586: Misleading Indentation check

2017-02-14 Thread Gábor Horváth via Phabricator via cfe-commits 
xazax.hun updated this revision to Diff 88330.
xazax.hun marked 7 inline comments as done.
xazax.hun added a comment.

- Added a note to make it easier to understand the diagnostics.
- Reworded the error message about dangling else.
- Fixed other review comments.


https://reviews.llvm.org/D19586

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/MisleadingIndentationCheck.cpp
  clang-tidy/readability/MisleadingIndentationCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-misleading-indentation.rst
  test/clang-tidy/readability-misleading-indentation.cpp

Index: test/clang-tidy/readability-misleading-indentation.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-misleading-indentation.cpp
@@ -0,0 +1,80 @@
+// RUN: %check_clang_tidy %s readability-misleading-indentation %t
+
+void foo1();
+void foo2();
+
+#define BLOCK \
+  if (cond1)  \
+foo1();   \
+foo2();
+
+int main()
+{
+  bool cond1 = true;
+  bool cond2 = true;
+
+  if (cond1)
+if (cond2)
+  foo1();
+  else
+foo2();
+  // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: different indentation for 'if' and corresponding 'else' [readability-misleading-indentation]
+
+  if (cond1) {
+if (cond2)
+  foo1();
+  }
+  else
+foo2();
+
+  if (cond1)
+if (cond2)
+  foo1();
+else
+  foo2();
+
+  if (cond2)
+foo1();
+foo2();
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: misleading indentation: statement is indented too deeply [readability-misleading-indentation]
+// CHECK-MESSAGES: :[[@LINE-4]]:3: note: did you mean this line to be inside this 'if'
+foo2(); // No redundant warning.
+
+  if (cond1)
+  {
+foo1();
+  }
+foo2();
+
+  if (cond1)
+foo1();
+  foo2();
+
+  if (cond2)
+if (cond1) foo1(); else foo2();
+
+  if (cond1) {
+  } else {
+  }
+
+  if (cond1) {
+  }
+  else {
+  }
+
+  if (cond1)
+  {
+  }
+  else
+  {
+  }
+
+  if (cond1)
+{
+}
+  else
+{
+}
+
+  BLOCK
+}
Index: docs/clang-tidy/checks/readability-misleading-indentation.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/readability-misleading-indentation.rst
@@ -0,0 +1,38 @@
+.. title:: clang-tidy - readability-misleading-indentation
+
+readability-misleading-indentation
+==
+
+Correct indentation helps to understand code. Mismatch of the syntactical
+structure and the indentation of the code may hide serious problems.
+Missing braces can also make it significantly harder to read the code,
+therefore it is important to use braces. 
+
+The way to avoid dangling else is to always check that an ``else`` belongs
+to the ``if`` that begins in the same column.
+
+You can omit braces when your inner part of e.g. an ``if`` statement has only
+one statement in it. Although in that case you should begin the next statement
+in the same column with the ``if``.
+
+Examples:
+
+.. code-block:: c++
+
+  // Dangling else:
+  if (cond1)
+if (cond2)
+  foo1();
+  else
+foo2();  // Wrong indentation: else belongs to if(cond2) statement.
+
+  // Missing braces:
+  if (cond1)
+foo1();
+foo2();  // Not guarded by if(cond1).
+
+Limitations
+===
+
+Note that this check only works as expected when the tabs or spaces are used
+consistently and not mixed.
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -139,6 +139,7 @@
readability-identifier-naming
readability-implicit-bool-cast
readability-inconsistent-declaration-parameter-name
+   readability-misleading-indentation
readability-misplaced-array-index
readability-named-parameter
readability-non-const-parameter
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -57,6 +57,11 @@
 Improvements to clang-tidy
 --
 
+- New `readability-misleading-indentation
+  `_ check
+
+  Finds misleading indentation where braces should be introduced or the code should be reformatted.
+
 - New `safety-no-assembler
   `_ check
 
Index: clang-tidy/readability/ReadabilityTidyModule.cpp
===
--- clang-tidy/readability/ReadabilityTidyModule.cpp
+++ clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -20,6 +20,7 @@
 #include "IdentifierNamingCheck.h"
 #include "ImplicitBoolCastCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"
+#include "MisleadingIndentationCheck.h"
 #include

[PATCH] D19586: Misleading Indentation check

2017-02-13 Thread Daniel Marjamäki via Phabricator via cfe-commits 
danielmarjamaki added inline comments.



Comment at: clang-tidy/readability/MisleadingIndentationCheck.cpp:42
+const Stmt *Inside = nullptr;
+
+if (const auto *CurrentIf = dyn_cast(CurrentStmt)) {

I would rename Inside to Inner. That would make InnerLoc more "consistent".



Comment at: clang-tidy/readability/MisleadingIndentationCheck.cpp:48
+  Inside = CurrentFor->getBody();
+} else if (const auto CurrentWhile = dyn_cast(CurrentStmt)) {
+  Inside = CurrentWhile->getBody();

nit: write "const auto *CurrentWhile...". missing "*"?




Comment at: test/clang-tidy/readability-misleading-indentation.cpp:21
+foo2();
+  // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: potential dangling 'else' 
[readability-misleading-indentation]
+

I am skeptic about this warning message.

Why does it say "potential". I would say that in this test case the indentation 
_is_ "dangling".

The message is not very clear to me. I personally don't intuitively understand 
what is wrong without looking at the code.

I don't know what it should say. Maybe:
```
different indentation for 'if' and 'else'
```



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] D19586: Misleading Indentation check

2017-02-13 Thread Alexander Kornienko via Phabricator via cfe-commits 
alexfh requested changes to this revision.
alexfh added inline comments.
This revision now requires changes to proceed.



Comment at: clang-tidy/readability/MisleadingIndentationCheck.cpp:72
+SM.getExpansionColumnNumber(NextLoc))
+  diag(NextLoc, "misleading indentation: statement is indented too 
deeply");
+  }

Will it be useful to add a note pointing to the control statement and saying 
"did you mean this line to be inside this if/while/for/..."?



Comment at: clang-tidy/readability/MisleadingIndentationCheck.cpp:79
+  Finder->addMatcher(
+  compoundStmt(anyOf(has(ifStmt()), has(forStmt()), has(whileStmt(
+  .bind("compound"),

`has(anyOf(ifStmt(), forStmt(), whileStmt()))` would read better.


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] D19586: Misleading Indentation check

2017-02-13 Thread Gábor Horváth via Phabricator via cfe-commits 
xazax.hun marked 13 inline comments as done.
xazax.hun added inline comments.



Comment at: clang-tidy/readability/MisleadingIndentationCheck.cpp:20
+
+void MisleadingIndentationCheck::danglingElseCheck(
+const MatchFinder::MatchResult ) {

danielmarjamaki wrote:
> 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?
Documented this limitation.


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] D19586: Misleading Indentation check

2017-02-13 Thread Gábor Horváth via Phabricator via cfe-commits 
xazax.hun updated this revision to Diff 88182.
xazax.hun added a comment.

- Updated to latest trunk.
- Mentioned check in the release notes.
- Documented the limitation that tabs and spaces need to be consistent for this 
check to work.
- Fixed (hopefully all) review comments.
- Fixed the test cases.
- Minor code cleanups.


https://reviews.llvm.org/D19586

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/MisleadingIndentationCheck.cpp
  clang-tidy/readability/MisleadingIndentationCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-misleading-indentation.rst
  test/clang-tidy/readability-misleading-indentation.cpp

Index: test/clang-tidy/readability-misleading-indentation.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-misleading-indentation.cpp
@@ -0,0 +1,79 @@
+// RUN: %check_clang_tidy %s readability-misleading-indentation %t
+
+void foo1();
+void foo2();
+
+#define BLOCK \
+  if (cond1)  \
+foo1();   \
+foo2();
+
+int main()
+{
+  bool cond1 = true;
+  bool cond2 = true;
+
+  if (cond1)
+if (cond2)
+  foo1();
+  else
+foo2();
+  // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: potential dangling 'else' [readability-misleading-indentation]
+
+  if (cond1) {
+if (cond2)
+  foo1();
+  }
+  else
+foo2();
+
+  if (cond1)
+if (cond2)
+  foo1();
+else
+  foo2();
+
+  if (cond2)
+foo1();
+foo2();
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: misleading indentation: statement is indented too deeply [readability-misleading-indentation]
+foo2(); // No redundant warning.
+
+  if (cond1)
+  {
+foo1();
+  }
+foo2();
+
+  if (cond1)
+foo1();
+  foo2();
+
+  if (cond2)
+if (cond1) foo1(); else foo2();
+
+  if (cond1) {
+  } else {
+  }
+
+  if (cond1) {
+  }
+  else {
+  }
+
+  if (cond1)
+  {
+  }
+  else
+  {
+  }
+
+  if (cond1)
+{
+}
+  else
+{
+}
+
+  BLOCK
+}
Index: docs/clang-tidy/checks/readability-misleading-indentation.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/readability-misleading-indentation.rst
@@ -0,0 +1,38 @@
+.. title:: clang-tidy - readability-misleading-indentation
+
+readability-misleading-indentation
+==
+
+Correct indentation helps to understand code. Mismatch of the syntactical
+structure and the indentation of the code may hide serious problems.
+Missing braces can also make it significantly harder to read the code,
+therefore it is important to use braces. 
+
+The way to avoid dangling else is to always check that an ``else`` belongs
+to the ``if`` that begins in the same column.
+
+You can omit braces when your inner part of e.g. an ``if`` statement has only
+one statement in it. Although in that case you should begin the next statement
+in the same column with the ``if``.
+
+Examples:
+
+.. code-block:: c++
+
+  // Dangling else:
+  if (cond1)
+if (cond2)
+  foo1();
+  else
+foo2();  // Wrong indentation: else belongs to if(cond2) statement.
+
+  // Missing braces:
+  if (cond1)
+foo1();
+foo2();  // Not guarded by if(cond1).
+
+Limitations
+===
+
+Note that this check only works as expected when the tabs or spaces are used
+consistently and not mixed.
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -139,6 +139,7 @@
readability-identifier-naming
readability-implicit-bool-cast
readability-inconsistent-declaration-parameter-name
+   readability-misleading-indentation
readability-misplaced-array-index
readability-named-parameter
readability-non-const-parameter
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -57,6 +57,11 @@
 Improvements to clang-tidy
 --
 
+- New `readability-misleading-indentation
+  `_ check
+
+  Finds misleading indentation where braces should be introduced or the code should be reformatted.
+
 - New `safety-no-assembler
   `_ check
 
Index: clang-tidy/readability/ReadabilityTidyModule.cpp
===
--- clang-tidy/readability/ReadabilityTidyModule.cpp
+++ clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -20,6 +20,7 @@
 #include "IdentifierNamingCheck.h"
 #include "ImplicitBoolCastCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"
+#include "MisleadingIndentationCheck.h"
 #include "MisplacedArrayIndexCheck.h"
 #include "NamedParameterCheck.h"
 #include

[PATCH] D19586: Misleading Indentation check

2017-02-13 Thread Gábor Horváth via Phabricator via cfe-commits 
xazax.hun commandeered this revision.
xazax.hun edited reviewers, added: Pajesz; removed: xazax.hun.
xazax.hun added a comment.
Herald added a subscriber: mgorny.

The original author is no longer available.


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] D19586: Misleading Indentation check

2016-10-05 Thread Daniel Marjamäki via cfe-commits 
danielmarjamaki added inline comments.


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

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

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

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

It's unfortunate. Is it ok?

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

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

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

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

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

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

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

there are too many spaces in this comment

https://reviews.llvm.org/D19586



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

[PATCH] D19586: Misleading Indentation check

2016-10-04 Thread Pauer Gergely via cfe-commits 
Pajesz requested a review of this revision.
Pajesz added reviewers: dkrupp, xazax.hun, nlewycky, etienne.bergeron, etienneb.
Pajesz marked an inline comment as done.
Pajesz added a comment.

Hello!

Gonna fix the tests ASAP! Any other suggestions, fixes, improvements 
considering the checker?


https://reviews.llvm.org/D19586



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

Re: [PATCH] D19586: Misleading Indentation check

2016-09-27 Thread Gábor Horváth via cfe-commits 
xazax.hun added inline comments.


Comment at: test/clang-tidy/readability-misleading-indentation.cpp:16
@@ +15,3 @@
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: wrong indentation, 
'else' belongs to 'if(cond2)' statement
+  // CHECK-FIXES: {{^}}  // comment1
+

Pajesz wrote:
> alexfh wrote:
> > Did you run the tests after changes? I don't think the `// comment1` line 
> > can actually appear in the fixed code, when it's not there before the fix.
> Could you please explain what check-fixes does and how? Im not sure about it.
You can read the documentation here: 
http://clang.llvm.org/extra/clang-tidy/#testing-checks

Didn't you execute the tests? (Using make check-all or some similar command)

The CHECK-FIXES line will check the content of the file after the fixits are 
applied. The check will fail when the specified pattern is not found in the 
result.


https://reviews.llvm.org/D19586



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

Re: [PATCH] D19586: Misleading Indentation check

2016-07-19 Thread Alexander Kornienko via cfe-commits 
alexfh added a comment.

Please mark all addressed comments "Done".



Comment at: test/clang-tidy/readability-misleading-indentation.cpp:15
@@ +14,3 @@
+  foo2(); // comment
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: wrong indentation, 
'else' belongs to 'if(cond2)' statement
+  // CHECK-FIXES: {{^}}  // comment1

Please place // CHECK comments at column 1 or 3. Indenting them like this makes 
the test less readable.


Comment at: test/clang-tidy/readability-misleading-indentation.cpp:16
@@ +15,3 @@
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: wrong indentation, 
'else' belongs to 'if(cond2)' statement
+  // CHECK-FIXES: {{^}}  // comment1
+

Did you run the tests after changes? I don't think the `// comment1` line can 
actually appear in the fixed code, when it's not there before the fix.


https://reviews.llvm.org/D19586



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

Re: [PATCH] D19586: Misleading Indentation check

2016-07-14 Thread Pauer Gergely via cfe-commits 
Pajesz updated this revision to Diff 63925.
Pajesz added a comment.

Minor changes in tests and doc and diff should be full now.


http://reviews.llvm.org/D19586

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/MisleadingIndentationCheck.cpp
  clang-tidy/readability/MisleadingIndentationCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-misleading-indentation.rst
  test/clang-tidy/readability-misleading-indentation.cpp

Index: test/clang-tidy/readability-misleading-indentation.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-misleading-indentation.cpp
@@ -0,0 +1,130 @@
+// RUN: %check_clang_tidy %s readability-misleading-indentation %t
+
+void foo1() {}
+void foo2() {}
+
+int main() {
+  bool cond1 = true;
+  bool cond2 = true;
+
+  if (cond1)
+if (cond2)
+  foo1();
+else
+  foo2(); // comment
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: wrong indentation, 'else' belongs to 'if(cond2)' statement
+  // CHECK-FIXES: {{^}}  // comment1
+
+  if (cond1) {
+if (cond2)
+  foo1();
+  } else
+foo2(); // ok, 'else' belongs to 'if(cond1)' statement
+
+  if (cond1)
+if (cond2)
+  foo1();
+else
+  foo2(); // ok, indentation matches to syntactical structure
+
+  if (cond1)
+foo1();
+  foo2(); // comment
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: not part of 'if(cond1)'
+  // CHECK-FIXES: {{^}}  // comment2
+
+  if (cond2)
+foo1();
+  foo2(); // comment
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: not part of 'if(cond2)'
+  // CHECK-FIXES: {{^}}  // comment3
+
+  if (cond1) {
+foo1();
+  }
+  foo2(); // ok
+
+  if (cond1)
+foo1();
+  foo2(); // ok
+
+  if (cond1)
+foo1();
+  foo2(); // comment
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: not part of 'if(cond1)'
+  // CHECK-FIXES: {{^}}  // comment4
+  foo2(); // no need for redundant warning
+
+  if (cond1) { // ok
+foo1();
+  } else {
+foo2();
+  }
+
+  if (cond1)
+foo1();
+  else
+foo2(); // ok
+
+  if (cond1) { // ok
+foo1();
+  } else {
+foo2();
+  }
+
+  if (cond1) // ok
+  {
+foo1();
+  } else {
+foo2();
+  }
+
+  if (cond1) // ok
+  {
+foo1();
+  } else {
+foo2();
+  }
+
+  while (cond1) {
+foo1();
+  }
+  foo2(); // ok
+
+  while (cond1)
+foo1();
+  foo2(); // comment
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: not part of 'while(cond1)'
+  // CHECK-FIXES: {{^}}  // comment5
+
+  while (cond1) {
+foo1();
+  }
+  foo2(); // ok
+
+  while (cond1)
+foo1();
+  foo2(); // ok
+
+  for (;;) {
+foo1();
+  }
+  foo2(); // ok
+
+  for (;;)
+foo1();
+  foo2(); // comment
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: not part of 'for(;;)'
+  // CHECK-FIXES: {{^}}  // comment6
+
+  for (;;) {
+foo1();
+  }
+  foo2(); // ok
+
+  for (;;)
+foo1();
+  foo2(); // ok
+
+  return 0;
+}
Index: docs/clang-tidy/checks/readability-misleading-indentation.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/readability-misleading-indentation.rst
@@ -0,0 +1,36 @@
+.. title:: clang-tidy - readability-misleading-indentation
+
+readability-misleading-indentation
+==
+
+Description
+
+Correct indentation helps to understand code. Mismatch of the syntactical
+structure and the indentation of the code may reveal serious problems.
+Missing braces can also cause some problems in analyzing the code,
+therefore it is important to use braces. 
+
+The way to avoid dangling else is to always check that an ``else`` belongs
+to the ``if`` that begins in the same column.
+
+You can omit braces when your inner part of e.g. an ``if`` statement has only
+one statement in it. Although in that case you should begin the next statement
+in the same column with the ``if``.
+
+Examples:
+
+.. code-block:: c++
+
+  // Dangling else:
+
+  if (cond1)
+if (cond2)
+  foo1();
+  else
+foo2();  // wrong indentation: else belongs to if(cond2) statement
+
+  // Missing braces
+
+  if (cond1)
+foo1();
+foo2();  // not part of if(cond1)
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -93,6 +93,7 @@
readability-identifier-naming
readability-implicit-bool-cast
readability-inconsistent-declaration-parameter-name
+   readability-misleading-indentation
readability-named-parameter
readability-redundant-control-flow
readability-redundant-smartptr-get
Index: clang-tidy/readability/ReadabilityTidyModule.cpp
===
--- clang-tidy/readability/ReadabilityTidyModule.cpp
+++

Re: [PATCH] D19586: Misleading Indentation check

2016-07-06 Thread Alexander Kornienko via cfe-commits 
alexfh added a comment.

Gergely, it seems that the last diff is missing 
clang-tidy/readability/MisleadingIndentationCheck.cpp. A few more comments 
below.



Comment at: docs/clang-tidy/checks/readability-misleading-indentation.rst:13
@@ +12,3 @@
+
+The way to avoid dangling else is to always check that an "else" belongs
+to the "if" that begins in the same column.

nit: Please use double backquotes to mark inline code fragments.


Comment at: test/clang-tidy/readability-misleading-indentation.cpp:16
@@ +15,3 @@
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: wrong indentation, else belongs 
to
+  // if(cond2) statement
+  // CHECK-FIXES: {{^}}  // comment

1. This is not a part of the previous CHECK-MESSAGES. Is it intended? It's fine 
to have long lines in tests.
2. Code snippets in the message should be enclosed in single quotes ('else', 
'if(cond2)' ...).
3. Please specify each unique message once completely (including the 
[check-name] part).


Comment at: test/clang-tidy/readability-misleading-indentation.cpp:17
@@ +16,3 @@
+  // if(cond2) statement
+  // CHECK-FIXES: {{^}}  // comment
+

These `CHECK-FIXES:` are totally unreliable, since they are all matching the 
same pattern. FileCheck (which is used to verify the output against these CHECK 
lines) maintains no implicit relation between the check line location and the 
line number matched in the output. The two things that matter to FileCheck are 
the order of the matched lines and their content. So the patterns need to be 
unique to avoid matches to incorrect lines. Change them to, e.g. `// comment1`, 
`// comment2`, ...


Comment at: test/clang-tidy/readability-misleading-indentation.cpp:63
@@ +62,3 @@
+foo2();
+  }
+

What about this comment?


Comment at: test/clang-tidy/readability-misleading-indentation.cpp:63
@@ +62,3 @@
+foo2();
+  }
+

alexfh wrote:
> What about this comment?
What about this comment?


http://reviews.llvm.org/D19586



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

Re: [PATCH] D19586: Misleading Indentation check

2016-06-23 Thread Pauer Gergely via cfe-commits 
Pajesz updated this revision to Diff 61651.
Pajesz added a comment.

Checker now works with for and while statements as well, new tests were added, 
other syntactical and logical updates have been made.


http://reviews.llvm.org/D19586

Files:
  clang-tidy/readability/MisleadingIndentationCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-misleading-indentation.rst
  test/clang-tidy/readability-misleading-indentation.cpp

Index: test/clang-tidy/readability-misleading-indentation.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-misleading-indentation.cpp
@@ -0,0 +1,131 @@
+// RUN: %check_clang_tidy %s readability-misleading-indentation %t
+
+void foo1() {}
+void foo2() {}
+
+int main() {
+  bool cond1 = true;
+  bool cond2 = true;
+
+  if (cond1)
+if (cond2)
+  foo1();
+else
+  foo2(); // comment
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: wrong indentation, else belongs to
+  // if(cond2) statement
+  // CHECK-FIXES: {{^}}  // comment
+
+  if (cond1) {
+if (cond2)
+  foo1();
+  } else
+foo2(); // ok, else belongs to if(cond1) statement
+
+  if (cond1)
+if (cond2)
+  foo1();
+else
+  foo2(); // ok, indentation matches to syntactical structure
+
+  if (cond1)
+foo1();
+  foo2(); // comment
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: not part of if(cond1)
+  // CHECK-FIXES: {{^}}  // comment
+
+  if (cond2)
+foo1();
+  foo2(); // comment
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: not part of if(cond2)
+  // CHECK-FIXES: {{^}}  // comment
+
+  if (cond1) {
+foo1();
+  }
+  foo2(); // ok
+
+  if (cond1)
+foo1();
+  foo2(); // ok
+
+  if (cond1)
+foo1();
+  foo2(); // comment
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: not part of if(cond1)
+  // CHECK-FIXES: {{^}}  // comment
+  foo2(); // no need for redundant warning
+
+  if (cond1) { // ok
+foo1();
+  } else {
+foo2();
+  }
+
+  if (cond1)
+foo1();
+  else
+foo2(); // ok
+
+  if (cond1) { // ok
+foo1();
+  } else {
+foo2();
+  }
+
+  if (cond1) // ok
+  {
+foo1();
+  } else {
+foo2();
+  }
+
+  if (cond1) // ok
+  {
+foo1();
+  } else {
+foo2();
+  }
+
+  while (cond1) {
+foo1();
+  }
+  foo2(); // ok
+
+  while (cond1)
+foo1();
+  foo2(); // comment
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: not part of while(cond1)
+  // CHECK-FIXES: {{^}}  // comment
+
+  while (cond1) {
+foo1();
+  }
+  foo2(); // ok
+
+  while (cond1)
+foo1();
+  foo2(); // ok
+
+  for (;;) {
+foo1();
+  }
+  foo2(); // ok
+
+  for (;;)
+foo1();
+  foo2(); // comment
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: not part of for(;;)
+  // CHECK-FIXES: {{^}}  // comment
+
+  for (;;) {
+foo1();
+  }
+  foo2(); // ok
+
+  for (;;)
+foo1();
+  foo2(); // ok
+
+  return 0;
+}
Index: docs/clang-tidy/checks/readability-misleading-indentation.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/readability-misleading-indentation.rst
@@ -0,0 +1,36 @@
+.. title:: clang-tidy - readability-misleading-indentation
+
+readability-misleading-indentation
+==
+
+Description
+
+Correct indentation helps to understand code. Mismatch of the syntactical
+structure and the indentation of the code may reveal serious problems.
+Missing braces can also cause some problems in analyzing the code,
+therefore it is important to use braces. 
+
+The way to avoid dangling else is to always check that an "else" belongs
+to the "if" that begins in the same column.
+
+You can omit braces when your inner part of e.g. an "if" statement has only
+one statement in it. Although in that case you should begin the next statement
+in the same column with the "if".
+
+Examples:
+
+.. code-block:: c++
+
+  // Dangling else:
+
+  if (cond1)
+if (cond2)
+  foo1();
+  else
+foo2();  // wrong indentation: else belongs to if(cond2) statement
+
+  // Missing braces
+
+  if (cond1)
+foo1();
+foo2();  // not part of if(cond1)
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -93,6 +93,7 @@
readability-identifier-naming
readability-implicit-bool-cast
readability-inconsistent-declaration-parameter-name
+   readability-misleading-indentation
readability-named-parameter
readability-redundant-control-flow
readability-redundant-smartptr-get
Index: clang-tidy/readability/ReadabilityTidyModule.cpp
===
--- clang-tidy/readability/ReadabilityTidyModule.cpp
+++ clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -17,6 +17,7 @@
 #include

Re: [PATCH] D19586: Misleading Indentation check

2016-04-29 Thread Etienne Bergeron via cfe-commits 
etienneb added a comment.

In http://reviews.llvm.org/D19586#417063, @xazax.hun wrote:

> In http://reviews.llvm.org/D19586#417053, @etienneb wrote:
>
> > The rule also apply for statements in a same compound:
> >
> >   {
> > statement1();
> > statement2();
> >   statement3();
> >
> >
> > But this can be a further improvement.
>
>
> I believe this might be an intentional omission, since this can not imply 
> semantic problems. But I agree that, this addition makes sense.


Fair enough. This checker is in 'readbility', so I don't see why not. 
But, feel free to postpone. Or let someone else take it.


http://reviews.llvm.org/D19586



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

Re: [PATCH] D19586: Misleading Indentation check

2016-04-29 Thread Gábor Horváth via cfe-commits 
xazax.hun added a comment.

In http://reviews.llvm.org/D19586#417053, @etienneb wrote:

> The rule also apply for statements in a same compound:
>
>   {
> statement1();
> statement2();
>   statement3();
>
>
> But this can be a further improvement.


I believe this might be an intentional omission, since this can not imply 
semantic problems. But I agree that, this addition makes sense.


http://reviews.llvm.org/D19586



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

Re: [PATCH] D19586: Misleading Indentation check

2016-04-29 Thread Etienne Bergeron via cfe-commits 
etienneb added a comment.

You should not limit the checker to "if".
The for-statement and while-statement are also wrong for the same reasons:

  while (x)
statement1();
statement2();



  for (...)
statement1();
statement2();

You should test your code over large code base.
You should catch these cases:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/frame/FrameView.cpp=setMediaType=package:chromium=cs=1253

You will also find strange cases like:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/icu/source/i18n/usearch.cpp=initializepattern=package:chromium=cs=438

The rule also apply for statements in a same compound:

  {
statement1();
statement2();
  statement3();

see: 
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/icu/source/common/ushape.cpp=third_party/icu/source/common/ushape.cpp=package:chromium=cs=784

But this can be a further improvement.


http://reviews.llvm.org/D19586



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

Re: [PATCH] D19586: Misleading Indentation check

2016-04-29 Thread Etienne Bergeron via cfe-commits 
etienneb requested changes to this revision.
etienneb added a comment.
This revision now requires changes to proceed.

Could you lift some logics to the matcher instead of the check.



Comment at: clang-tidy/readability/MisleadingIndentationCheck.cpp:22
@@ +21,3 @@
+const MatchFinder::MatchResult ) {
+  auto *If = Result.Nodes.getNodeAs("if");
+  if (!If->getElse())

nit: const


Comment at: clang-tidy/readability/MisleadingIndentationCheck.cpp:40
@@ +39,3 @@
+const MatchFinder::MatchResult ) {
+  auto *CStmt = Result.Nodes.getNodeAs("stmt");
+  for (unsigned int i = 0; i < CStmt->size() - 1; i++) {

nit: const


Comment at: clang-tidy/readability/MisleadingIndentationCheck.cpp:67
@@ +66,3 @@
+  diag(NextLoc, "Wrong Indentation - statement is indented as a member "
+"of if statement");
+  }

of if statement  -> previous if statement


Comment at: clang-tidy/readability/MisleadingIndentationCheck.cpp:72
@@ +71,3 @@
+void MisleadingIndentationCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(ifStmt().bind("if"), this);
+  Finder->addMatcher(compoundStmt(has(ifStmt())).bind("stmt"), this);

instead of doing check at line 23:
```
   if (!If->getElse())
```

you should use 'hasElse' matcher here.

As I get it, you want to match something like:

```
anyOf(
  ifStatement(hasThen(stmt().bind("last")),
 hasElse(unless(compoundStmt())),
  ifStatement(hasThen(unless(compoundStmt())),
 unless(hasElse(stmt().bind("last")))
)
```

Then, by getting node named "last" you can retrieve the indentation of the last 
statement.




Comment at: clang-tidy/readability/MisleadingIndentationCheck.cpp:77
@@ +76,3 @@
+void MisleadingIndentationCheck::check(const MatchFinder::MatchResult ) 
{
+
+  if (Result.Nodes.getNodeAs("if"))

nit: remove empty line


Comment at: clang-tidy/readability/MisleadingIndentationCheck.h:30
@@ +29,3 @@
+  void check(const ast_matchers::MatchFinder::MatchResult ) override;
+  void danglingElseCheck(const ast_matchers::MatchFinder::MatchResult );
+  void missingBracesCheck(const ast_matchers::MatchFinder::MatchResult 
);

nit: private


Comment at: docs/clang-tidy/checks/readability-misleading-indentation.rst:8
@@ +7,3 @@
+
+Correct indentation helps to understand code. Mismatch of the syntactical 
structure and the indentation of the code may reveal serious pr
+

could you fix these long lines.


Comment at: test/clang-tidy/readability-misleading-indentation.cpp:62
@@ +61,2 @@
+   return 0;
+}

I would like to see more tests with { }

```
if (xxx) {
} else {
}
```

```
if (xxx) {
}
else {
}
```

```
if (xxx)
{
}
else
{
}
```

```
if (xxx)
  {
  }
else
  {
  }
```


Comment at: test/clang-tidy/readability-misleading-indentation.cpp:62
@@ +61,2 @@
+   return 0;
+}

etienneb wrote:
> I would like to see more tests with { }
> 
> ```
> if (xxx) {
> } else {
> }
> ```
> 
> ```
> if (xxx) {
> }
> else {
> }
> ```
> 
> ```
> if (xxx)
> {
> }
> else
> {
> }
> ```
> 
> ```
> if (xxx)
>   {
>   }
> else
>   {
>   }
> ```
could you add test with macro:

```
#define BLOCK
  if (xxx)\
stm1();  \
stm2();
```


http://reviews.llvm.org/D19586



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

Re: [PATCH] D19586: Misleading Indentation check

2016-04-28 Thread Pauer Gergely via cfe-commits 
Pajesz updated this revision to Diff 55389.
Pajesz marked 5 inline comments as done.
Pajesz added a comment.

Both dangling else and the other check now ignore one-line if-else statements. 
Corrected other reviews as well.


http://reviews.llvm.org/D19586

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/MisleadingIndentationCheck.cpp
  clang-tidy/readability/MisleadingIndentationCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-misleading-indentation.rst
  test/clang-tidy/readability-misleading-indentation.cpp

Index: test/clang-tidy/readability-misleading-indentation.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-misleading-indentation.cpp
@@ -0,0 +1,62 @@
+// RUN: %check_clang_tidy %s readability-misleading-indentation %t
+
+void foo1() {}
+void foo2() {}
+
+int main()
+{
+   bool cond1 = true;
+   bool cond2 = true;
+
+   if (cond1)
+  if (cond2)
+ foo1();
+   else
+ foo2();  // comment
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: wrong indentation, else belongs to if(cond2) statement
+// CHECK-FIXES: {{^}}  // comment
+
+   if (cond1) {
+  if (cond2)
+ foo1();
+   }
+   else
+  foo2();  // ok, else belongs to if(cond1) statement
+
+   if (cond1)
+ if (cond2)
+foo1();
+ else
+foo2();  // ok, indentation matches to syntactical structure
+
+   if (cond1)
+  foo1();
+  foo2(); // comment
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: not part of if(cond1)
+// CHECK-FIXES: {{^}}  // comment
+
+   if (cond2)
+  foo1();
+  foo2(); // comment
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: not part of if(cond2)
+// CHECK-FIXES: {{^}}  // comment
+
+   if (cond1)
+   {
+  foo1();
+   }
+  foo2();  // ok
+
+   if (cond1)
+  foo1();
+   foo2();  // ok
+
+  if (cond1)
+  foo1();
+  foo2(); // comment
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: not part of if(cond1)
+// CHECK-FIXES: {{^}}  // comment
+  foo2(); // no need for redundant warning
+
+   return 0;
+}
Index: docs/clang-tidy/checks/readability-misleading-indentation.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/readability-misleading-indentation.rst
@@ -0,0 +1,30 @@
+.. title:: clang-tidy - readability-misleading-indentation
+
+readability-misleading-indentation
+==
+
+Description
+
+Correct indentation helps to understand code. Mismatch of the syntactical structure and the indentation of the code may reveal serious pr
+
+The way to avoid dangling else is to always check that an "else" belongs to the "if" that begins in the same column.
+
+You can omit braces when your inner part of e.g. an "if" statement has only one statement in it. Although in that case you should begin t
+
+Examples:
+
+.. code-block:: c++
+
+  // Dangling else:
+
+  if (cond1)
+if (cond2)
+  foo1();
+  else
+foo2();  // wrong indentation: else belongs to if(cond2) statement
+
+  // Missing braces
+
+  if (cond1)
+foo1();
+foo2();  // not part of if(cond1)
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -93,6 +93,7 @@
readability-identifier-naming
readability-implicit-bool-cast
readability-inconsistent-declaration-parameter-name
+   readability-misleading-indentation
readability-named-parameter
readability-redundant-control-flow
readability-redundant-smartptr-get
Index: clang-tidy/readability/ReadabilityTidyModule.cpp
===
--- clang-tidy/readability/ReadabilityTidyModule.cpp
+++ clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -17,6 +17,7 @@
 #include "IdentifierNamingCheck.h"
 #include "ImplicitBoolCastCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"
+#include "MisleadingIndentationCheck.h"
 #include "NamedParameterCheck.h"
 #include "RedundantControlFlowCheck.h"
 #include "RedundantSmartptrGetCheck.h"
@@ -45,6 +46,8 @@
 "readability-implicit-bool-cast");
 CheckFactories.registerCheck(
 "readability-inconsistent-declaration-parameter-name");
+CheckFactories.registerCheck(
+"readability-misleading-indentation");
 CheckFactories.registerCheck(
 "readability-named-parameter");
 CheckFactories.registerCheck(
Index: clang-tidy/readability/MisleadingIndentationCheck.h
===
--- /dev/null
+++ clang-tidy/readability/MisleadingIndentationCheck.h
@@ -0,0 +1,38 @@
+//===--- MisleadingIndentationCheck.h - clang-tidy---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois

Re: [PATCH] D19586: Misleading Indentation check

2016-04-28 Thread Gábor Horváth via cfe-commits 
xazax.hun added a comment.

In http://reviews.llvm.org/D19586#415123, @Pajesz wrote:

> Both dangling else and the other check now ignore one-line if-else 
> statements. Corrected other reviews as well.


I can not see a test case for one line if-then-else. Could you add it?


http://reviews.llvm.org/D19586



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

Re: [PATCH] D19586: Misleading Indentation check

2016-04-27 Thread Nick Lewycky via cfe-commits 
nlewycky added a subscriber: nlewycky.


Comment at: clang-tidy/readability/MisleadingIndentationCheck.cpp:31
@@ +30,3 @@
+  Result.SourceManager->getExpansionColumnNumber(ElseLoc))
+diag(ElseLoc, "potentional dangling else");
+}

Typo of "potential" as "potentional".


Comment at: clang-tidy/readability/MisleadingIndentationCheck.cpp:54
@@ +53,3 @@
+
+if (Result.SourceManager->getExpansionColumnNumber(StmtLoc) ==
+Result.SourceManager->getExpansionColumnNumber(NextLoc))

What about a one-line "if (x) foo(); else bar();"? Warn on it anyways?


Comment at: clang-tidy/readability/MisleadingIndentationCheck.cpp:56
@@ +55,3 @@
+Result.SourceManager->getExpansionColumnNumber(NextLoc))
+  diag(NextLoc, "Wrong Indentation - statement is indentated as a member "
+"of if statement");

Typo of "indented" as "indentated".


Comment at: test/clang-tidy/readability-misleading-indentation.cpp:48
@@ +47,3 @@
+   }
+  foo2();  // ok
+

This is arguably misleading indentation, but I'm ok with not warning on it if 
you think it will cause too many false positives.


http://reviews.llvm.org/D19586



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

Re: [PATCH] D19586: Misleading Indentation check

2016-04-27 Thread Eugene Zelenko via cfe-commits 
Eugene.Zelenko added a subscriber: Eugene.Zelenko.
Eugene.Zelenko added a comment.

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).

What about for ans while statements?



Comment at: docs/clang-tidy/checks:10
@@ +9,3 @@
+
+The way to avoid dangling else is to always check that an else belongs to the 
if that begins in th
+

Please highlight if/else with ``.


Comment at: docs/clang-tidy/checks:12
@@ +11,3 @@
+
+You can omit braces when your inner part of e.g. an IF statement has only one 
statement in it. Alt
+

Please use ``if'' instead of IF.


http://reviews.llvm.org/D19586



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

Re: [PATCH] D19586: Misleading Indentation check

2016-04-27 Thread Pauer Gergely via cfe-commits 
Pajesz updated this revision to Diff 55247.
Pajesz added a comment.

Probably fixed the broken patch.


http://reviews.llvm.org/D19586

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/MisleadingIndentationCheck.cpp
  clang-tidy/readability/MisleadingIndentationCheck.h
  clang-tidy/readability/ReadabilityTidyModule.cpp
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-misleading-indentation.r
  test/clang-tidy/readability-misleading-indentation.cpp

Index: test/clang-tidy/readability-misleading-indentation.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-misleading-indentation.cpp
@@ -0,0 +1,62 @@
+// RUN: %check_clang_tidy %s readability-misleading-indentation %t
+
+void foo1() {}
+void foo2() {}
+
+int main()
+{
+   bool cond1 = true;
+   bool cond2 = true;
+
+   if (cond1)
+  if (cond2)
+ foo1();
+   else
+ foo2();  // comment
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: wrong indentation, else belongs to if(cond2) statement
+// CHECK-FIXES: {{^}}  // comment
+
+   if (cond1) {
+  if (cond2)
+ foo1();
+   }
+   else
+  foo2();  // ok, else belongs to if(cond1) statement
+
+   if (cond1)
+ if (cond2)
+foo1();
+ else
+foo2();  // ok, indentation matches to syntactical structure
+
+   if (cond1)
+  foo1();
+  foo2(); // comment
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: not part of if(cond1)
+// CHECK-FIXES: {{^}}  // comment
+
+   if (cond2)
+  foo1();
+  foo2(); // comment
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: not part of if(cond2)
+// CHECK-FIXES: {{^}}  // comment
+
+   if (cond1)
+   {
+  foo1();
+   }
+  foo2();  // ok
+
+   if (cond1)
+  foo1();
+   foo2();  // ok
+
+  if (cond1)
+  foo1();
+  foo2(); // comment
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: not part of if(cond1)
+// CHECK-FIXES: {{^}}  // comment
+  foo2(); // no need for redundant warning
+
+   return 0;
+}
Index: docs/clang-tidy/checks/readability-misleading-indentation.r
===
--- /dev/null
+++ docs/clang-tidy/checks/readability-misleading-indentation.r
@@ -0,0 +1,30 @@
+.. title:: clang-tidy - readability-misleading-indentation
+
+readability-misleading-indentation
+==
+
+Description
+
+Correct indentation helps to understand code. Mismatch of the syntactical structure and the indentation of the code may reveal serious 
+
+The way to avoid dangling else is to always check that an else belongs to the if that begins in the same column.
+
+You can omit braces when your inner part of e.g. an IF statement has only one statement in it. Although in that case you should begin t
+
+Examples:
+
+.. code-block:: c++
+
+  // Dangling else:
+
+  if (cond1)
+if (cond2)
+  foo1();
+  else
+foo2();  // wrong indentation: else belongs to if(cond2) statement
+
+  // Missing braces
+
+  if (cond1)
+foo1();
+foo2();  // not part of if(cond1)
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -93,6 +93,7 @@
readability-identifier-naming
readability-implicit-bool-cast
readability-inconsistent-declaration-parameter-name
+   readability-misleading-indentation
readability-named-parameter
readability-redundant-control-flow
readability-redundant-smartptr-get
Index: clang-tidy/readability/ReadabilityTidyModule.cpp
===
--- clang-tidy/readability/ReadabilityTidyModule.cpp
+++ clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -17,6 +17,7 @@
 #include "IdentifierNamingCheck.h"
 #include "ImplicitBoolCastCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"
+#include "MisleadingIndentationCheck.h"
 #include "NamedParameterCheck.h"
 #include "RedundantControlFlowCheck.h"
 #include "RedundantSmartptrGetCheck.h"
@@ -45,6 +46,8 @@
 "readability-implicit-bool-cast");
 CheckFactories.registerCheck(
 "readability-inconsistent-declaration-parameter-name");
+CheckFactories.registerCheck(
+"readability-misleading-indentation");
 CheckFactories.registerCheck(
 "readability-named-parameter");
 CheckFactories.registerCheck(
Index: clang-tidy/readability/MisleadingIndentationCheck.h
===
--- /dev/null
+++ clang-tidy/readability/MisleadingIndentationCheck.h
@@ -0,0 +1,38 @@
+//===--- MisleadingIndentationCheck.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.
+//

Re: [PATCH] D19586: Misleading Indentation check

2016-04-27 Thread Alexander Kornienko via cfe-commits 
alexfh added a comment.

The patch is broken:

  Index: clang-tidy/readability/Mislead
  ===
  --- /dev/null
  +++ clang-tidy/readability/Mislead
  @@ -0,0 +1,77 @@
  +//===--- MisleadingIndentationCheck.cpp - 
clang-tidy---===//

Please fix and re-upload.


http://reviews.llvm.org/D19586



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

[PATCH] D19586: Misleading Indentation check

2016-04-27 Thread Pauer Gergely via cfe-commits 
Pajesz created this revision.
Pajesz added a reviewer: alexfh.
Pajesz added subscribers: cfe-commits, xazax.hun.

This is a clang-tidy check for llvm. It checks for dangling else and for 
possible misleading indentation due to omitted braces. 


http://reviews.llvm.org/D19586

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/Mislead
  clang-tidy/readability/Misleadin
  clang-tidy/readability/ReadabilityT
  docs/clang-tidy/checks
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/readability-m

Index: test/clang-tidy/readability-m
===
--- /dev/null
+++ test/clang-tidy/readability-m
@@ -0,0 +1,62 @@
+// RUN: %check_clang_tidy %s readability-misleading-indentation %t
+
+void foo1() {}
+void foo2() {}
+
+int main()
+{
+   bool cond1 = true;
+   bool cond2 = true;
+
+   if (cond1)
+  if (cond2)
+ foo1();
+   else
+ foo2();  // comment
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: wrong indentation, else belongs to if(cond2) statement
+// CHECK-FIXES: {{^}}  // comment
+
+   if (cond1) {
+  if (cond2)
+ foo1();
+   }
+   else
+  foo2();  // ok, else belongs to if(cond1) statement
+
+   if (cond1)
+ if (cond2)
+foo1();
+ else
+foo2();  // ok, indentation matches to syntactical structure
+
+   if (cond1)
+  foo1();
+  foo2(); // comment
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: not part of if(cond1)
+// CHECK-FIXES: {{^}}  // comment
+
+   if (cond2)
+  foo1();
+  foo2(); // comment
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: not part of if(cond2)
+// CHECK-FIXES: {{^}}  // comment
+
+   if (cond1)
+   {
+  foo1();
+   }
+  foo2();  // ok
+
+   if (cond1)
+  foo1();
+   foo2();  // ok
+
+  if (cond1)
+  foo1();
+  foo2(); // comment
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: not part of if(cond1)
+// CHECK-FIXES: {{^}}  // comment
+  foo2(); // no need for redundant warning
+
+   return 0;
+}
Index: docs/clang-tidy/checks
===
--- /dev/null
+++ docs/clang-tidy/checks
@@ -0,0 +1,30 @@
+.. title:: clang-tidy - readability-misleading-indentation
+
+readability-misleading-indentation
+==
+
+Description
+
+Correct indentation helps to understand code. Mismatch of the syntactical structure and the indent
+
+The way to avoid dangling else is to always check that an else belongs to the if that begins in th
+
+You can omit braces when your inner part of e.g. an IF statement has only one statement in it. Alt
+
+Examples:
+
+.. code-block:: c++
+
+  // Dangling else:
+
+  if (cond1)
+if (cond2)
+  foo1();
+  else
+foo2();  // wrong indentation: else belongs to if(cond2) statement
+
+  // Missing braces
+
+  if (cond1)
+foo1();
+foo2();  // not part of if(cond1)
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -93,6 +93,7 @@
readability-identifier-naming
readability-implicit-bool-cast
readability-inconsistent-declaration-parameter-name
+   readability-misleading-indentation
readability-named-parameter
readability-redundant-control-flow
readability-redundant-smartptr-get
Index: clang-tidy/readability/ReadabilityT
===
--- clang-tidy/readability/ReadabilityT
+++ clang-tidy/readability/ReadabilityT
@@ -17,6 +17,7 @@
 #include "IdentifierNamingCheck.h"
 #include "ImplicitBoolCastCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"
+#include "MisleadingIndentationCheck.h"
 #include "NamedParameterCheck.h"
 #include "RedundantControlFlowCheck.h"
 #include "RedundantSmartptrGetCheck.h"
@@ -45,6 +46,8 @@
 "readability-implicit-bool-cast");
 CheckFactories.registerCheck(
 "readability-inconsistent-declaration-parameter-name");
+CheckFactories.registerCheck(
+"readability-misleading-indentation");
 CheckFactories.registerCheck(
 "readability-named-parameter");
 CheckFactories.registerCheck(
Index: clang-tidy/readability/Misleadin
===
--- /dev/null
+++ clang-tidy/readability/Misleadin
@@ -0,0 +1,38 @@
+//===--- MisleadingIndentationCheck.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_MISLEADING_INDENTATION_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_MISLEADING_INDENTATION_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+///