[PATCH] D70638: [Diagnostic] add a warning which warns about misleading indentation

[Dave Lee via Phabricator via cfe-commits]

kastiglione added a comment.

@Tyker in case you didn't see my previous message, I'm curious if you'd be 
willing to take a look at the bug. Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70638/new/

https://reviews.llvm.org/D70638

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

[PATCH] D70638: [Diagnostic] add a warning which warns about misleading indentation

[Dave Lee via Phabricator via cfe-commits]

kastiglione added a comment.

@Tyker Hi, I noticed a case that isn't caught by `-Wmisleading-indentation`. In 
code that uses two space indents, there's a corner case that isn't caught when 
the preceding `if` uses curly braces. I've noticed a couple instances of this 
in lldb.

For example:

  if (cond) {
then1();
then2();
// ...
  } else
else1();
else2();

The `else2();` statement doesn't trigger the warning.

It seems that the logic is to use the column of the `else` keyword, not the 
column of the right brace. When using a four space indent (any indent != 2), 
the warning is emitted:

  } else
  else1();
  else2();


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70638/new/

https://reviews.llvm.org/D70638

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

[PATCH] D70638: [Diagnostic] add a warning which warns about misleading indentation

[Tyker via Phabricator via cfe-commits]

Tyker added a comment.

In D70638#1804138 , @aaron.ballman 
wrote:

> In D70638#1803364 , @xbolva00 wrote:
>
> > (re-ping; I think this false positive for goto label case is important to 
> > be fixed before 10 release)
>
>
> I agree -- @Tyker, are you planning to work on that fix?


i made a patch. https://reviews.llvm.org/D72202


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70638/new/

https://reviews.llvm.org/D70638



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

[PATCH] D70638: [Diagnostic] add a warning which warns about misleading indentation

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D70638#1803364 , @xbolva00 wrote:

> (re-ping; I think this false positive for goto label case is important to be 
> fixed before 10 release)


I agree -- @Tyker, are you planning to work on that fix?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70638/new/

https://reviews.llvm.org/D70638



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

[PATCH] D70638: [Diagnostic] add a warning which warns about misleading indentation

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

(re-ping; I think this false positive for goto label case is important to be 
fixed before 10 release)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70638/new/

https://reviews.llvm.org/D70638



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

[PATCH] D70638: [Diagnostic] add a warning which warns about misleading indentation

[Nathan Chancellor via Phabricator via cfe-commits]

nathanchance added a comment.

As a follow up to my previous post, I have sent patches to clean up all of the 
warnings that I see in the Linux kernel. However, I found one that I do believe 
is a false positive:

  ../drivers/staging/uwb/allocator.c:353:3: warning: misleading indentation; 
statement is not part of the previous 'else' [-Wmisleading-indentation]
alloc_found:
^
  ../drivers/staging/uwb/allocator.c:350:2: note: previous statement is here
  else
  ^
  1 warning generated.

Corresponding to 
https://github.com/torvalds/linux/blob/2187f215ebaac73ddbd814696d7c7fa34f0c3de0/drivers/staging/uwb/allocator.c#L346-L353.

Simplified:

  $ cat test.c
  int a(int b, int c) {
if (b)
goto label;
  
if (c)
return 0;
  
label:
return 1;
  }
  
  $ clang -Wmisleading-indentation -o /dev/null -c test.c
  test.c:8:3: warning: misleading indentation; statement is not part of the 
previous 'if' [-Wmisleading-indentation]
label:
^
  test.c:5:2: note: previous statement is here
  if (c)
  ^
  1 warning generated.

goto labels are unaffected by indentation so there should be no warning here. 
While I think that the labels should be unindented for style, the driver is 
marked as obsolete and is scheduled to be deleted so I am not sure such a patch 
would be welcomed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70638/new/

https://reviews.llvm.org/D70638



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

[PATCH] D70638: [Diagnostic] add a warning which warns about misleading indentation

[Tyker via Phabricator via cfe-commits]

Tyker added a comment.

i have not tested the performance impact. but i tried to do the heavier checks 
last to minimize the impact.
i added your tests file to the improvement commit.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70638/new/

https://reviews.llvm.org/D70638



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

[PATCH] D70638: [Diagnostic] add a warning which warns about misleading indentation

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

Also, did you check if this has a measurable impact on compilation speed?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70638/new/

https://reviews.llvm.org/D70638



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

[PATCH] D70638: [Diagnostic] add a warning which warns about misleading indentation

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

Hey, cool! I gave this a shot myself many years ago at 
http://llvm.org/bugs/show_bug.cgi?id=18938 That bug has a list of interesting 
test cases I had collected back then. Maybe you want to check and see how this 
does on those cases?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70638/new/

https://reviews.llvm.org/D70638



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

[PATCH] D70638: [Diagnostic] add a warning which warns about misleading indentation

[Nathan Chancellor via Phabricator via cfe-commits]

nathanchance added a comment.

> IMO that is misleading indentation. A single space followed by a tab? WTF

I guess I will take a look over all of the warnings that cropped up and see if 
this is the case for all of them. If it is, I will just send patches to fix 
those (with the justification of the warning and the fact that it would be a 
checkpatch warning), rather than adding `-ftabstop` to `KBUILD_CFLAGS`.

  $ ./scripts/checkpatch.pl --terse --types LEADING_SPACE -f 
drivers/video/fbdev/core/fbmem.c
  drivers/video/fbdev/core/fbmem.c:665: WARNING: please, no spaces at the start 
of a line
  drivers/video/fbdev/core/fbmem.c:666: WARNING: please, no spaces at the start 
of a line
  drivers/video/fbdev/core/fbmem.c:667: WARNING: please, no spaces at the start 
of a line
  drivers/video/fbdev/core/fbmem.c:668: WARNING: please, no spaces at the start 
of a line
  drivers/video/fbdev/core/fbmem.c:669: WARNING: please, no spaces at the start 
of a line
  drivers/video/fbdev/core/fbmem.c:670: WARNING: please, no spaces at the start 
of a line
  drivers/video/fbdev/core/fbmem.c:671: WARNING: please, no spaces at the start 
of a line
  drivers/video/fbdev/core/fbmem.c:672: WARNING: please, no spaces at the start 
of a line
  drivers/video/fbdev/core/fbmem.c:673: WARNING: please, no spaces at the start 
of a line
  drivers/video/fbdev/core/fbmem.c:674: WARNING: please, no spaces at the start 
of a line
  drivers/video/fbdev/core/fbmem.c:675: WARNING: please, no spaces at the start 
of a line
  drivers/video/fbdev/core/fbmem.c:676: WARNING: please, no spaces at the start 
of a line
  drivers/video/fbdev/core/fbmem.c:677: WARNING: please, no spaces at the start 
of a line
  drivers/video/fbdev/core/fbmem.c:678: WARNING: please, no spaces at the start 
of a line
  drivers/video/fbdev/core/fbmem.c:1063: WARNING: please, no spaces at the 
start of a line
  drivers/video/fbdev/core/fbmem.c:1064: WARNING: please, no spaces at the 
start of a line
  drivers/video/fbdev/core/fbmem.c:1070: WARNING: please, no spaces at the 
start of a line
  drivers/video/fbdev/core/fbmem.c:1075: WARNING: please, no spaces at the 
start of a line


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70638/new/

https://reviews.llvm.org/D70638



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

[PATCH] D70638: [Diagnostic] add a warning which warns about misleading indentation

[Tyker via Phabricator via cfe-commits]

Tyker added a comment.

the warning was not affected by -ftabstop. i made a patch that fix this.

https://reviews.llvm.org/D71037

and I agree that mixing tabs and space is a horrible idea for many reasons.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70638/new/

https://reviews.llvm.org/D70638



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

[PATCH] D70638: [Diagnostic] add a warning which warns about misleading indentation

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

In D70638#1768200 , @nathanchance 
wrote:

> As an FYI, this appears to cause several false positive warnings with the 
> Linux kernel:
>
>   ../drivers/video/fbdev/core/fbmem.c:665:3: warning: misleading indentation; 
> statement is not part of the previous 'else' [-Wmisleading-indentation]
>   if (fb_logo.depth > 4 && depth > 4) {
>   ^
>   ../drivers/video/fbdev/core/fbmem.c:661:2: note: previous statement is here
>   else
>   ^
>   ../drivers/video/fbdev/core/fbmem.c:1075:3: warning: misleading 
> indentation; statement is not part of the previous 'if' 
> [-Wmisleading-indentation]
>   return ret;
>   ^
>   ../drivers/video/fbdev/core/fbmem.c:1072:2: note: previous statement is here
>   if (!ret)
>   ^
>   2 warnings generated.
>
>
> Corresponding to:
>
> https://elixir.bootlin.com/linux/v5.4.1/source/drivers/video/fbdev/core/fbmem.c#L665
>
> and
>
> https://elixir.bootlin.com/linux/v5.4.1/source/drivers/video/fbdev/core/fbmem.c#L1072


IMO that is misleading indentation. A single space followed by a tab? WTF


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70638/new/

https://reviews.llvm.org/D70638



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

[PATCH] D70638: [Diagnostic] add a warning which warns about misleading indentation

[Tyker via Phabricator via cfe-commits]

Tyker marked 6 inline comments as done.
Tyker added a comment.

i did a few test on the linux kernel on prior version of this patchs and the 
mix of spaces and tabs caused some false positives. but i do believe there is 
still a bug here.
for the mix of space and tabs. gcc has a -ftabstop=//X// to specify how large 
tabs should be counted as.
clang has it as well i am going to check that it is taken in account.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70638/new/

https://reviews.llvm.org/D70638



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

[PATCH] D70638: [Diagnostic] add a warning which warns about misleading indentation

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added inline comments.



Comment at: clang/test/Parser/warn-misleading-indentation.cpp:209
+}
\ No newline at end of file


Please add newline.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70638/new/

https://reviews.llvm.org/D70638



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

[PATCH] D70638: [Diagnostic] add a warning which warns about misleading indentation

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

fbmem.c case reduced:

https://godbolt.org/z/ma4aFA

Looks like kernel uses tabs here

So we have BinaryOperator 

If we use spaces: BinaryOperator  'int' '='


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70638/new/

https://reviews.llvm.org/D70638



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

[PATCH] D70638: [Diagnostic] add a warning which warns about misleading indentation

[Nathan Chancellor via Phabricator via cfe-commits]

nathanchance added a comment.

As an FYI, this appears to cause several false positive warnings with the Linux 
kernel:

  ../drivers/video/fbdev/core/fbmem.c:665:3: warning: misleading indentation; 
statement is not part of the previous 'else' [-Wmisleading-indentation]
  if (fb_logo.depth > 4 && depth > 4) {
  ^
  ../drivers/video/fbdev/core/fbmem.c:661:2: note: previous statement is here
  else
  ^
  ../drivers/video/fbdev/core/fbmem.c:1075:3: warning: misleading indentation; 
statement is not part of the previous 'if' [-Wmisleading-indentation]
  return ret;
  ^
  ../drivers/video/fbdev/core/fbmem.c:1072:2: note: previous statement is here
  if (!ret)
  ^
  2 warnings generated.

Corresponding to:

https://elixir.bootlin.com/linux/v5.4.1/source/drivers/video/fbdev/core/fbmem.c#L665

and

https://elixir.bootlin.com/linux/v5.4.1/source/drivers/video/fbdev/core/fbmem.c#L1072


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70638/new/

https://reviews.llvm.org/D70638



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

[PATCH] D70638: [Diagnostic] add a warning which warns about misleading indentation

[Tyker via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGbc840b21e161: [Diagnostic] add a warning which warns about 
misleading indentation (authored by Tyker).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70638/new/

https://reviews.llvm.org/D70638

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Lex/Preprocessor.h
  clang/include/clang/Parse/Parser.h
  clang/lib/Parse/ParseStmt.cpp
  clang/test/Index/pragma-diag-reparse.c
  clang/test/Misc/warning-wall.c
  clang/test/Parser/warn-misleading-indentation.cpp
  clang/test/Preprocessor/pragma_diagnostic_sections.cpp

Index: clang/test/Preprocessor/pragma_diagnostic_sections.cpp
===
--- clang/test/Preprocessor/pragma_diagnostic_sections.cpp
+++ clang/test/Preprocessor/pragma_diagnostic_sections.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -Wall -Wunused-macros -Wunused-parameter -Wno-uninitialized -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wall -Wunused-macros -Wunused-parameter -Wno-uninitialized -Wno-misleading-indentation -verify %s
 
 // rdar://8365684
 struct S {
Index: clang/test/Parser/warn-misleading-indentation.cpp
===
--- /dev/null
+++ clang/test/Parser/warn-misleading-indentation.cpp
@@ -0,0 +1,208 @@
+// RUN: %clang_cc1 -x c -fsyntax-only -verify %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wmisleading-indentation -DWITH_WARN %s
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify -Wall -Wno-unused -DWITH_WARN -DCXX17 %s
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify -Wall -Wno-unused -Wno-misleading-indentation -DCXX17 %s
+
+#ifndef WITH_WARN
+// expected-no-diagnostics
+#endif
+
+void f0(int i) {
+  if (i)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = i + 1;
+int x = 0;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+#endif
+  return;
+#ifdef CXX17
+  if constexpr (false)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = 0;
+i += 1;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+#endif
+#endif
+}
+
+void f1(int i) {
+  for (;i;)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = i + 1;
+i *= 2;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'for'}}
+#endif
+  return;
+}
+
+void f2(int i) {
+  while (i)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = i + 1; i *= 2;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'while'}}
+#endif
+  return;
+}
+
+void f3(int i) {
+  if (i)
+i = i + 1;
+  else
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i *= 2;
+const int x = 0;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'else'}}
+#endif
+}
+
+#ifdef CXX17
+struct Range {
+  int *begin() {return nullptr;}
+  int *end() {return nullptr;}
+};
+#endif
+
+void f4(int i) {
+  if (i)
+  i *= 2;
+  return;
+  if (i)
+i *= 2;
+;
+  if (i)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i *= 2;
+typedef int Int;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+#endif
+#ifdef CXX17
+  Range R;
+  for (auto e : R)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i *= 2;
+using Int2 = int;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'for'}}
+#endif
+#endif
+}
+
+int bar(void);
+
+int foo(int* dst)
+{   
+if (dst)
+   return
+bar();
+  if (dst)
+dst = dst + \
+bar();
+  return 0;
+}
+
+void g(int i) {
+  if (1)
+i = 2;
+  else
+ if (i == 3)
+#ifdef WITH_WARN
+// expected-note@-3 {{here}}
+#endif
+i = 4;
+i = 5;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+#endif
+}
+
+// Or this
+#define TEST i = 5
+void g0(int i) {
+  if (1)
+i = 2;
+  else
+i = 5;
+TEST;
+}
+
+void g1(int i) {
+  if (1)
+i = 2;
+  else if (i == 3)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+  i = 4;
+  i = 5;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+#endif
+}
+
+void g2(int i) {
+  if (1)
+i = 2;
+  else
+if (i == 3)
+{i = 4;}
+i = 5;
+}
+
+void g6(int i) {
+if (1)
+if (i == 3)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = 4;
+i = 5;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; 

[PATCH] D70638: [Diagnostic] add a warning which warns about misleading indentation

[Tyker via Phabricator via cfe-commits]

Tyker updated this revision to Diff 231949.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70638/new/

https://reviews.llvm.org/D70638

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Lex/Preprocessor.h
  clang/include/clang/Parse/Parser.h
  clang/lib/Parse/ParseStmt.cpp
  clang/test/Index/pragma-diag-reparse.c
  clang/test/Misc/warning-wall.c
  clang/test/Parser/warn-misleading-indentation.cpp
  clang/test/Preprocessor/pragma_diagnostic_sections.cpp

Index: clang/test/Preprocessor/pragma_diagnostic_sections.cpp
===
--- clang/test/Preprocessor/pragma_diagnostic_sections.cpp
+++ clang/test/Preprocessor/pragma_diagnostic_sections.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -Wall -Wunused-macros -Wunused-parameter -Wno-uninitialized -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wall -Wunused-macros -Wunused-parameter -Wno-uninitialized -Wno-misleading-indentation -verify %s
 
 // rdar://8365684
 struct S {
Index: clang/test/Parser/warn-misleading-indentation.cpp
===
--- /dev/null
+++ clang/test/Parser/warn-misleading-indentation.cpp
@@ -0,0 +1,208 @@
+// RUN: %clang_cc1 -x c -fsyntax-only -verify %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wmisleading-indentation -DWITH_WARN %s
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify -Wall -Wno-unused -DWITH_WARN -DCXX17 %s
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify -Wall -Wno-unused -Wno-misleading-indentation -DCXX17 %s
+
+#ifndef WITH_WARN
+// expected-no-diagnostics
+#endif
+
+void f0(int i) {
+  if (i)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = i + 1;
+int x = 0;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+#endif
+  return;
+#ifdef CXX17
+  if constexpr (false)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = 0;
+i += 1;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+#endif
+#endif
+}
+
+void f1(int i) {
+  for (;i;)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = i + 1;
+i *= 2;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'for'}}
+#endif
+  return;
+}
+
+void f2(int i) {
+  while (i)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = i + 1; i *= 2;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'while'}}
+#endif
+  return;
+}
+
+void f3(int i) {
+  if (i)
+i = i + 1;
+  else
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i *= 2;
+const int x = 0;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'else'}}
+#endif
+}
+
+#ifdef CXX17
+struct Range {
+  int *begin() {return nullptr;}
+  int *end() {return nullptr;}
+};
+#endif
+
+void f4(int i) {
+  if (i)
+  i *= 2;
+  return;
+  if (i)
+i *= 2;
+;
+  if (i)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i *= 2;
+typedef int Int;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+#endif
+#ifdef CXX17
+  Range R;
+  for (auto e : R)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i *= 2;
+using Int2 = int;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'for'}}
+#endif
+#endif
+}
+
+int bar(void);
+
+int foo(int* dst)
+{   
+if (dst)
+   return
+bar();
+  if (dst)
+dst = dst + \
+bar();
+  return 0;
+}
+
+void g(int i) {
+  if (1)
+i = 2;
+  else
+ if (i == 3)
+#ifdef WITH_WARN
+// expected-note@-3 {{here}}
+#endif
+i = 4;
+i = 5;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+#endif
+}
+
+// Or this
+#define TEST i = 5
+void g0(int i) {
+  if (1)
+i = 2;
+  else
+i = 5;
+TEST;
+}
+
+void g1(int i) {
+  if (1)
+i = 2;
+  else if (i == 3)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+  i = 4;
+  i = 5;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+#endif
+}
+
+void g2(int i) {
+  if (1)
+i = 2;
+  else
+if (i == 3)
+{i = 4;}
+i = 5;
+}
+
+void g6(int i) {
+if (1)
+if (i == 3)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = 4;
+i = 5;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+#endif
+}
+
+void g7(int i) {
+  if (1)
+i = 4;
+#ifdef TEST1
+#endif
+i = 5;
+}
+
+void a1(int i) { if (1) i = 4; return; }
+
+void a2(int i) {
+ 

[PATCH] D70638: [Diagnostic] add a warning which warns about misleading indentation

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM with some minor nits.




Comment at: clang/lib/Parse/ParseStmt.cpp:1219
+  void Check() {
+
+NeedsChecking = false;

Spurious whitespace, but it would be useful to add a newline above the 
`Check()` declaration for visual separation.



Comment at: clang/lib/Parse/ParseStmt.cpp:1239
+ !Tok.isAtStartOfLine())) {
+  if (SM.getPresumedLineNumber(StmtLoc) ==
+  SM.getPresumedLineNumber(Tok.getLocation()))

This can be hoisted (negated) into the preceding `if` statement.



Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2532
+if (Val.getAsLocSymbol() == Sym) {
+  const VarRegion *VR = MR->getBaseRegion()->getAs();
+  // Do not show local variables belonging to a function other than

xbolva00 wrote:
> Please land as separated NFCI commit.
Yeah, these changes seem unrelated to the patch.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70638/new/

https://reviews.llvm.org/D70638



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

[PATCH] D70638: [Diagnostic] add a warning which warns about misleading indentation

[Tyker via Phabricator via cfe-commits]

Tyker updated this revision to Diff 231792.
Tyker added a comment.

improved based on aaron's comment.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70638/new/

https://reviews.llvm.org/D70638

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Lex/Preprocessor.h
  clang/include/clang/Parse/Parser.h
  clang/lib/Parse/ParseStmt.cpp
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/test/Index/pragma-diag-reparse.c
  clang/test/Misc/warning-wall.c
  clang/test/Parser/warn-misleading-indentation.cpp
  clang/test/Preprocessor/pragma_diagnostic_sections.cpp

Index: clang/test/Preprocessor/pragma_diagnostic_sections.cpp
===
--- clang/test/Preprocessor/pragma_diagnostic_sections.cpp
+++ clang/test/Preprocessor/pragma_diagnostic_sections.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -Wall -Wunused-macros -Wunused-parameter -Wno-uninitialized -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wall -Wunused-macros -Wunused-parameter -Wno-uninitialized -Wno-misleading-indentation -verify %s
 
 // rdar://8365684
 struct S {
Index: clang/test/Parser/warn-misleading-indentation.cpp
===
--- /dev/null
+++ clang/test/Parser/warn-misleading-indentation.cpp
@@ -0,0 +1,208 @@
+// RUN: %clang_cc1 -x c -fsyntax-only -verify %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wmisleading-indentation -DWITH_WARN %s
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify -Wall -Wno-unused -DWITH_WARN -DCXX17 %s
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify -Wall -Wno-unused -Wno-misleading-indentation -DCXX17 %s
+
+#ifndef WITH_WARN
+// expected-no-diagnostics
+#endif
+
+void f0(int i) {
+  if (i)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = i + 1;
+int x = 0;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+#endif
+  return;
+#ifdef CXX17
+  if constexpr (false)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = 0;
+i += 1;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+#endif
+#endif
+}
+
+void f1(int i) {
+  for (;i;)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = i + 1;
+i *= 2;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'for'}}
+#endif
+  return;
+}
+
+void f2(int i) {
+  while (i)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = i + 1; i *= 2;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'while'}}
+#endif
+  return;
+}
+
+void f3(int i) {
+  if (i)
+i = i + 1;
+  else
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i *= 2;
+const int x = 0;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'else'}}
+#endif
+}
+
+#ifdef CXX17
+struct Range {
+  int *begin() {return nullptr;}
+  int *end() {return nullptr;}
+};
+#endif
+
+void f4(int i) {
+  if (i)
+  i *= 2;
+  return;
+  if (i)
+i *= 2;
+;
+  if (i)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i *= 2;
+typedef int Int;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+#endif
+#ifdef CXX17
+  Range R;
+  for (auto e : R)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i *= 2;
+using Int2 = int;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'for'}}
+#endif
+#endif
+}
+
+int bar(void);
+
+int foo(int* dst)
+{   
+if (dst)
+   return
+bar();
+  if (dst)
+dst = dst + \
+bar();
+  return 0;
+}
+
+void g(int i) {
+  if (1)
+i = 2;
+  else
+ if (i == 3)
+#ifdef WITH_WARN
+// expected-note@-3 {{here}}
+#endif
+i = 4;
+i = 5;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+#endif
+}
+
+// Or this
+#define TEST i = 5
+void g0(int i) {
+  if (1)
+i = 2;
+  else
+i = 5;
+TEST;
+}
+
+void g1(int i) {
+  if (1)
+i = 2;
+  else if (i == 3)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+  i = 4;
+  i = 5;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+#endif
+}
+
+void g2(int i) {
+  if (1)
+i = 2;
+  else
+if (i == 3)
+{i = 4;}
+i = 5;
+}
+
+void g6(int i) {
+if (1)
+if (i == 3)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = 4;
+i = 5;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+#endif
+}
+
+void g7(int i) {
+  if (1)

[PATCH] D70638: [Diagnostic] add a warning which warns about misleading indentation

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Parse/ParseStmt.cpp:1369-1370
 /*ShouldEnter=*/ConstexprCondition && *ConstexprCondition);
-ElseStmt = ParseStatement();
+if (Tok.is(tok::kw_if))
+  ElseStmt = ParseIfStatement(nullptr, ElseLoc);
+else

This looks incorrect to me. Consider a case like:
```
if (0) {
} else [[gsl::suppress("foo")]] if (1) {
}
```
I'm a little uneasy calling anything but `ParseStatement()` here as that is 
what needs to be parsed at this stage.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70638/new/

https://reviews.llvm.org/D70638



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

[PATCH] D70638: [Diagnostic] add a warning which warns about misleading indentation

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

Looks better, thanks.

Aaron ?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70638/new/

https://reviews.llvm.org/D70638



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

[PATCH] D70638: [Diagnostic] add a warning which warns about misleading indentation

[Tyker via Phabricator via cfe-commits]

Tyker updated this revision to Diff 231585.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70638/new/

https://reviews.llvm.org/D70638

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Lex/Preprocessor.h
  clang/include/clang/Parse/Parser.h
  clang/lib/Parse/ParseStmt.cpp
  clang/test/Index/pragma-diag-reparse.c
  clang/test/Misc/warning-wall.c
  clang/test/Parser/warn-misleading-indentation.cpp
  clang/test/Preprocessor/pragma_diagnostic_sections.cpp

Index: clang/test/Preprocessor/pragma_diagnostic_sections.cpp
===
--- clang/test/Preprocessor/pragma_diagnostic_sections.cpp
+++ clang/test/Preprocessor/pragma_diagnostic_sections.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -Wall -Wunused-macros -Wunused-parameter -Wno-uninitialized -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wall -Wunused-macros -Wunused-parameter -Wno-uninitialized -Wno-misleading-indentation -verify %s
 
 // rdar://8365684
 struct S {
Index: clang/test/Parser/warn-misleading-indentation.cpp
===
--- /dev/null
+++ clang/test/Parser/warn-misleading-indentation.cpp
@@ -0,0 +1,208 @@
+// RUN: %clang_cc1 -x c -fsyntax-only -verify %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wmisleading-indentation -DWITH_WARN %s
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify -Wall -Wno-unused -DWITH_WARN -DCXX17 %s
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify -Wall -Wno-unused -Wno-misleading-indentation -DCXX17 %s
+
+#ifndef WITH_WARN
+// expected-no-diagnostics
+#endif
+
+void f0(int i) {
+  if (i)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = i + 1;
+int x = 0;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+#endif
+  return;
+#ifdef CXX17
+  if constexpr (false)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = 0;
+i += 1;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+#endif
+#endif
+}
+
+void f1(int i) {
+  for (;i;)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = i + 1;
+i *= 2;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'for'}}
+#endif
+  return;
+}
+
+void f2(int i) {
+  while (i)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = i + 1; i *= 2;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'while'}}
+#endif
+  return;
+}
+
+void f3(int i) {
+  if (i)
+i = i + 1;
+  else
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i *= 2;
+const int x = 0;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'else'}}
+#endif
+}
+
+#ifdef CXX17
+struct Range {
+  int *begin() {return nullptr;}
+  int *end() {return nullptr;}
+};
+#endif
+
+void f4(int i) {
+  if (i)
+  i *= 2;
+  return;
+  if (i)
+i *= 2;
+;
+  if (i)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i *= 2;
+typedef int Int;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+#endif
+#ifdef CXX17
+  Range R;
+  for (auto e : R)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i *= 2;
+using Int2 = int;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'for'}}
+#endif
+#endif
+}
+
+int bar(void);
+
+int foo(int* dst)
+{   
+if (dst)
+   return
+bar();
+  if (dst)
+dst = dst + \
+bar();
+  return 0;
+}
+
+void g(int i) {
+  if (1)
+i = 2;
+  else
+ if (i == 3)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = 4;
+i = 5;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'else if'}}
+#endif
+}
+
+// Or this
+#define TEST i = 5
+void g0(int i) {
+  if (1)
+i = 2;
+  else
+i = 5;
+TEST;
+}
+
+void g1(int i) {
+  if (1)
+i = 2;
+  else if (i == 3)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+  i = 4;
+  i = 5;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'else if'}}
+#endif
+}
+
+void g2(int i) {
+  if (1)
+i = 2;
+  else
+if (i == 3)
+{i = 4;}
+i = 5;
+}
+
+void g6(int i) {
+if (1)
+if (i == 3)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = 4;
+i = 5;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+#endif
+}
+
+void g7(int i) {
+  if (1)
+i = 4;
+#ifdef TEST1
+#endif
+i = 5;
+}
+
+void a1(int i) { if (1) i = 4; return; }
+
+void 

[PATCH] D70638: [Diagnostic] add a warning which warns about misleading indentation

[Tyker via Phabricator via cfe-commits]

Tyker marked an inline comment as done.
Tyker added inline comments.



Comment at: clang/lib/Parse/ParseStmt.cpp:1376
+
+MIChecker.Check(ElseStmt.isUsable());
 

Tyker wrote:
> xbolva00 wrote:
> > What is wrong with code you used some rev ago?
> > 
> > if (usable) check();
> > 
> > Now you uselessly instantiate MIChecker since if Usable = false, check is 
> > not called.
> > 
> > I like the older code more...
> > 
> > If (usable) checkForMisleadingIndention(...) 
> > 
> > Was good and accepted.
> previous patch gave up on else if because we can't know wether there are 
> braces.
> 
> this revision can produce correct diagnostics on else if
the MisleadingIndentationChecker gathers information during its construction 
this allows having more context and removes many false positives. 

but I can bring back
If (usable) MIChecker.Check(...)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70638/new/

https://reviews.llvm.org/D70638



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

[PATCH] D70638: [Diagnostic] add a warning which warns about misleading indentation

[Tyker via Phabricator via cfe-commits]

Tyker marked an inline comment as done.
Tyker added inline comments.



Comment at: clang/lib/Parse/ParseStmt.cpp:1376
+
+MIChecker.Check(ElseStmt.isUsable());
 

xbolva00 wrote:
> What is wrong with code you used some rev ago?
> 
> if (usable) check();
> 
> Now you uselessly instantiate MIChecker since if Usable = false, check is not 
> called.
> 
> I like the older code more...
> 
> If (usable) checkForMisleadingIndention(...) 
> 
> Was good and accepted.
previous patch gave up on else if because we can't know wether there are braces.

this revision can produce correct diagnostics on else if


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70638/new/

https://reviews.llvm.org/D70638



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

[PATCH] D70638: [Diagnostic] add a warning which warns about misleading indentation

[Tyker via Phabricator via cfe-commits]

Tyker updated this revision to Diff 231584.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70638/new/

https://reviews.llvm.org/D70638

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Lex/Preprocessor.h
  clang/include/clang/Parse/Parser.h
  clang/lib/Parse/ParseStmt.cpp
  clang/test/Index/pragma-diag-reparse.c
  clang/test/Misc/warning-wall.c
  clang/test/Parser/warn-misleading-indentation.cpp
  clang/test/Preprocessor/pragma_diagnostic_sections.cpp

Index: clang/test/Preprocessor/pragma_diagnostic_sections.cpp
===
--- clang/test/Preprocessor/pragma_diagnostic_sections.cpp
+++ clang/test/Preprocessor/pragma_diagnostic_sections.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -Wall -Wunused-macros -Wunused-parameter -Wno-uninitialized -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wall -Wunused-macros -Wunused-parameter -Wno-uninitialized -Wno-misleading-indentation -verify %s
 
 // rdar://8365684
 struct S {
Index: clang/test/Parser/warn-misleading-indentation.cpp
===
--- /dev/null
+++ clang/test/Parser/warn-misleading-indentation.cpp
@@ -0,0 +1,208 @@
+// RUN: %clang_cc1 -x c -fsyntax-only -verify %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wmisleading-indentation -DWITH_WARN %s
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify -Wall -Wno-unused -DWITH_WARN -DCXX17 %s
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify -Wall -Wno-unused -Wno-misleading-indentation -DCXX17 %s
+
+#ifndef WITH_WARN
+// expected-no-diagnostics
+#endif
+
+void f0(int i) {
+  if (i)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = i + 1;
+int x = 0;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+#endif
+  return;
+#ifdef CXX17
+  if constexpr (false)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = 0;
+i += 1;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+#endif
+#endif
+}
+
+void f1(int i) {
+  for (;i;)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = i + 1;
+i *= 2;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'for'}}
+#endif
+  return;
+}
+
+void f2(int i) {
+  while (i)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = i + 1; i *= 2;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'while'}}
+#endif
+  return;
+}
+
+void f3(int i) {
+  if (i)
+i = i + 1;
+  else
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i *= 2;
+const int x = 0;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'else'}}
+#endif
+}
+
+#ifdef CXX17
+struct Range {
+  int *begin() {return nullptr;}
+  int *end() {return nullptr;}
+};
+#endif
+
+void f4(int i) {
+  if (i)
+  i *= 2;
+  return;
+  if (i)
+i *= 2;
+;
+  if (i)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i *= 2;
+typedef int Int;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+#endif
+#ifdef CXX17
+  Range R;
+  for (auto e : R)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i *= 2;
+using Int2 = int;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'for'}}
+#endif
+#endif
+}
+
+int bar(void);
+
+int foo(int* dst)
+{   
+if (dst)
+   return
+bar();
+  if (dst)
+dst = dst + \
+bar();
+  return 0;
+}
+
+void g(int i) {
+  if (1)
+i = 2;
+  else
+ if (i == 3)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = 4;
+i = 5;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'else if'}}
+#endif
+}
+
+// Or this
+#define TEST i = 5
+void g0(int i) {
+  if (1)
+i = 2;
+  else
+i = 5;
+TEST;
+}
+
+void g1(int i) {
+  if (1)
+i = 2;
+  else if (i == 3)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+  i = 4;
+  i = 5;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'else if'}}
+#endif
+}
+
+void g2(int i) {
+  if (1)
+i = 2;
+  else
+if (i == 3)
+{i = 4;}
+i = 5;
+}
+
+void g6(int i) {
+if (1)
+if (i == 3)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = 4;
+i = 5;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+#endif
+}
+
+void g7(int i) {
+  if (1)
+i = 4;
+#ifdef TEST1
+#endif
+i = 5;
+}
+
+void a1(int i) { if (1) i = 4; return; }
+
+void 

[PATCH] D70638: [Diagnostic] add a warning which warns about misleading indentation

[Tyker via Phabricator via cfe-commits]

Tyker updated this revision to Diff 231583.
Tyker added a comment.

Improve the warning for else if


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70638/new/

https://reviews.llvm.org/D70638

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Lex/Preprocessor.h
  clang/include/clang/Parse/Parser.h
  clang/lib/Parse/ParseStmt.cpp
  clang/test/Index/pragma-diag-reparse.c
  clang/test/Misc/warning-wall.c
  clang/test/Preprocessor/pragma_diagnostic_sections.cpp

Index: clang/test/Preprocessor/pragma_diagnostic_sections.cpp
===
--- clang/test/Preprocessor/pragma_diagnostic_sections.cpp
+++ clang/test/Preprocessor/pragma_diagnostic_sections.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -Wall -Wunused-macros -Wunused-parameter -Wno-uninitialized -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wall -Wunused-macros -Wunused-parameter -Wno-uninitialized -Wno-misleading-indentation -verify %s
 
 // rdar://8365684
 struct S {
Index: clang/test/Misc/warning-wall.c
===
--- clang/test/Misc/warning-wall.c
+++ clang/test/Misc/warning-wall.c
@@ -90,6 +90,7 @@
 CHECK-NEXT:-Wdangling-else
 CHECK-NEXT:  -Wswitch
 CHECK-NEXT:  -Wswitch-bool
+CHECK-NEXT:  -Wmisleading-indentation
 
 
 CHECK-NOT:-W
Index: clang/test/Index/pragma-diag-reparse.c
===
--- clang/test/Index/pragma-diag-reparse.c
+++ clang/test/Index/pragma-diag-reparse.c
@@ -11,6 +11,7 @@
   return x;
 }
 
+#pragma clang diagnostic ignored "-Wmisleading-indentation"
 void foo() { int b=0; while (b==b); }
 
 // RUN: env CINDEXTEST_EDITING=1 CINDEXTEST_FAILONERROR=1 c-index-test -test-load-source-reparse 5 local \
Index: clang/lib/Parse/ParseStmt.cpp
===
--- clang/lib/Parse/ParseStmt.cpp
+++ clang/lib/Parse/ParseStmt.cpp
@@ -1191,6 +1191,56 @@
   return false;
 }
 
+namespace {
+
+enum MisleadingStatementKind { MSK_if, MSK_else, MSK_for, MSK_while, MSK_else_if };
+
+struct MisleadingIndentationChecker {
+  Parser 
+  SourceLocation StmtLoc;
+  SourceLocation PrevLoc;
+  unsigned NumDirectives;
+  MisleadingStatementKind Kind;
+  bool NeedsChecking;
+  bool ShouldSkip;
+  MisleadingIndentationChecker(Parser , MisleadingStatementKind K,
+   SourceLocation SL)
+  : P(P), StmtLoc(SL), PrevLoc(P.getCurToken().getLocation()),
+NumDirectives(P.getPreprocessor().getNumDirectives()), Kind(K),
+NeedsChecking(true),
+ShouldSkip(P.getCurToken().is(tok::l_brace) ||
+   (K == MSK_else && P.getCurToken().is(tok::kw_if))) {}
+  void Check(bool ShouldCheck) {
+NeedsChecking = false;
+Token Tok = P.getCurToken();
+if (!ShouldCheck || ShouldSkip ||
+NumDirectives != P.getPreprocessor().getNumDirectives() ||
+Tok.isOneOf(tok::semi, tok::r_brace) || Tok.isAnnotation() ||
+Tok.getLocation().isMacroID() || PrevLoc.isMacroID() ||
+StmtLoc.isMacroID())
+  return;
+SourceManager  = P.getPreprocessor().getSourceManager();
+unsigned PrevColNum = SM.getSpellingColumnNumber(PrevLoc);
+unsigned CurColNum = SM.getSpellingColumnNumber(Tok.getLocation());
+unsigned StmtColNum = SM.getSpellingColumnNumber(StmtLoc);
+
+if (PrevColNum != 0 && CurColNum != 0 && StmtColNum != 0 &&
+((PrevColNum > StmtColNum && PrevColNum == CurColNum) ||
+ !Tok.isAtStartOfLine())) {
+  if (SM.getPresumedLineNumber(StmtLoc) ==
+  SM.getPresumedLineNumber(Tok.getLocation()))
+return;
+  P.Diag(Tok.getLocation(), diag::warn_misleading_indentation)
+  << Kind;
+  P.Diag(StmtLoc, diag::note_previous_statement);
+}
+  }
+  ~MisleadingIndentationChecker() {
+assert(!NeedsChecking && "Check Has not been called");
+  }
+};
+
+}
 
 /// ParseIfStatement
 ///   if-statement: [C99 6.8.4.1]
@@ -1199,7 +1249,8 @@
 /// [C++]   'if' '(' condition ')' statement
 /// [C++]   'if' '(' condition ')' statement 'else' statement
 ///
-StmtResult Parser::ParseIfStatement(SourceLocation *TrailingElseLoc) {
+StmtResult Parser::ParseIfStatement(SourceLocation *TrailingElseLoc,
+SourceLocation ElseLocOnElseIf) {
   assert(Tok.is(tok::kw_if) && "Not an if stmt!");
   SourceLocation IfLoc = ConsumeToken();  // eat the 'if'.
 
@@ -1265,6 +1316,10 @@
   //
   ParseScope InnerScope(this, Scope::DeclScope, C99orCXX, Tok.is(tok::l_brace));
 
+  MisleadingIndentationChecker MIChecker(
+  *this, ElseLocOnElseIf.isInvalid() ? MSK_if : MSK_else_if,
+  ElseLocOnElseIf.isInvalid() ? IfLoc : ElseLocOnElseIf);
+
   // Read the 'then' stmt.
   SourceLocation ThenStmtLoc = Tok.getLocation();
 
@@ -1278,6 +1333,8 @@
 ThenStmt = ParseStatement();
   }
 
+  

[PATCH] D70638: [Diagnostic] add a warning which warns about misleading indentation

[Tyker via Phabricator via cfe-commits]

Tyker updated this revision to Diff 231561.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70638/new/

https://reviews.llvm.org/D70638

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Lex/Preprocessor.h
  clang/lib/Parse/ParseStmt.cpp
  clang/test/Index/pragma-diag-reparse.c
  clang/test/Misc/warning-wall.c
  clang/test/Parser/warn-misleading-indentation.cpp
  clang/test/Preprocessor/pragma_diagnostic_sections.cpp

Index: clang/test/Preprocessor/pragma_diagnostic_sections.cpp
===
--- clang/test/Preprocessor/pragma_diagnostic_sections.cpp
+++ clang/test/Preprocessor/pragma_diagnostic_sections.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -Wall -Wunused-macros -Wunused-parameter -Wno-uninitialized -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wall -Wunused-macros -Wunused-parameter -Wno-uninitialized -Wno-misleading-indentation -verify %s
 
 // rdar://8365684
 struct S {
Index: clang/test/Parser/warn-misleading-indentation.cpp
===
--- /dev/null
+++ clang/test/Parser/warn-misleading-indentation.cpp
@@ -0,0 +1,210 @@
+// RUN: %clang_cc1 -x c -fsyntax-only -verify %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wmisleading-indentation -DWITH_WARN %s
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify -Wall -Wno-unused -DWITH_WARN -DCXX17 %s
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify -Wall -Wno-unused -Wno-misleading-indentation -DCXX17 %s
+
+#ifndef WITH_WARN
+// expected-no-diagnostics
+#endif
+
+void f0(int i) {
+  if (i)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = i + 1;
+int x = 0;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+#endif
+  return;
+#ifdef CXX17
+  if constexpr (false)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = 0;
+i += 1;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+#endif
+#endif
+}
+
+void f1(int i) {
+  for (;i;)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = i + 1;
+i *= 2;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'for'}}
+#endif
+  return;
+}
+
+void f2(int i) {
+  while (i)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = i + 1; i *= 2;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'while'}}
+#endif
+  return;
+}
+
+void f3(int i) {
+  if (i)
+i = i + 1;
+  else
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i *= 2;
+const int x = 0;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'else'}}
+#endif
+}
+
+#ifdef CXX17
+struct Range {
+  int *begin() {return nullptr;}
+  int *end() {return nullptr;}
+};
+#endif
+
+void f4(int i) {
+  if (i)
+  i *= 2;
+  return;
+  if (i)
+i *= 2;
+;
+  if (i)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i *= 2;
+typedef int Int;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+#endif
+#ifdef CXX17
+  Range R;
+  for (auto e : R)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i *= 2;
+using Int2 = int;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'for'}}
+#endif
+#endif
+}
+
+int bar(void);
+
+int foo(int* dst)
+{   
+if (dst)
+   return
+bar();
+  if (dst)
+dst = dst + \
+bar();
+  return 0;
+}
+
+// No diagnostics from GCC on this
+void g(int i) {
+  if (1)
+i = 2;
+  else
+ if (i == 3)
+i = 4;
+i = 5;
+}
+
+// Or this
+#define TEST i = 5
+void g0(int i) {
+  if (1)
+i = 2;
+  else
+i = 5;
+TEST;
+}
+
+void g1(int i) {
+  if (1)
+i = 2;
+  else
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+if (i == 3)
+i = 4;
+i = 5;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'else'}}
+#endif
+}
+
+void g2(int i) {
+  if (1)
+i = 2;
+  else
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+if (i == 3)
+{i = 4;}
+i = 5;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'else'}}
+#endif
+}
+
+void g6(int i) {
+if (1)
+if (i == 3)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = 4;
+i = 5;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+#endif
+}
+
+void g7(int i) {
+  if (1)
+i = 4;
+#ifdef TEST1
+#endif
+i = 5;
+}
+
+void a1(int i) { if (1) i = 4; return; }
+
+void a2(int i) 

[PATCH] D70638: [Diagnostic] add a warning which warns about misleading indentation

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

>> there is only one case in llvm's code in which this warning is more 
>> agressive thant GCC's.

I think that's okay. If after-commit feedback shows this is bad, we can always 
tune this diagnostic more and do not warn in that case.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70638/new/

https://reviews.llvm.org/D70638



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

[PATCH] D70638: [Diagnostic] add a warning which warns about misleading indentation

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added inline comments.



Comment at: clang/include/clang/Parse/Parser.h:2269
 
+public:
   /// isCXXDeclarationStatement - C++-specialized function that disambiguates

Extra change? Your code does not use isCXXDeclarationStatement.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70638/new/

https://reviews.llvm.org/D70638



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

[PATCH] D70638: [Diagnostic] add a warning which warns about misleading indentation

[Tyker via Phabricator via cfe-commits]

Tyker updated this revision to Diff 231545.
Tyker added a comment.

I just found out that `Parser::isCXXDeclarationStatement` is does more then 
just disambiguation it can emit diagnostics. which can cause error on correct 
code. so we can't use it in this context to disambiguate.

so it is not possible to easily disambiguate between statements and 
declaration. i changed the warning message to reflect that but the message 
isn't so good now.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70638/new/

https://reviews.llvm.org/D70638

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Lex/Preprocessor.h
  clang/include/clang/Parse/Parser.h
  clang/lib/Parse/ParseStmt.cpp
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/test/Index/pragma-diag-reparse.c
  clang/test/Misc/warning-wall.c
  clang/test/Preprocessor/pragma_diagnostic_sections.cpp

Index: clang/test/Preprocessor/pragma_diagnostic_sections.cpp
===
--- clang/test/Preprocessor/pragma_diagnostic_sections.cpp
+++ clang/test/Preprocessor/pragma_diagnostic_sections.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -Wall -Wunused-macros -Wunused-parameter -Wno-uninitialized -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wall -Wunused-macros -Wunused-parameter -Wno-uninitialized -Wno-misleading-indentation -verify %s
 
 // rdar://8365684
 struct S {
Index: clang/test/Misc/warning-wall.c
===
--- clang/test/Misc/warning-wall.c
+++ clang/test/Misc/warning-wall.c
@@ -90,6 +90,7 @@
 CHECK-NEXT:-Wdangling-else
 CHECK-NEXT:  -Wswitch
 CHECK-NEXT:  -Wswitch-bool
+CHECK-NEXT:  -Wmisleading-indentation
 
 
 CHECK-NOT:-W
Index: clang/test/Index/pragma-diag-reparse.c
===
--- clang/test/Index/pragma-diag-reparse.c
+++ clang/test/Index/pragma-diag-reparse.c
@@ -11,6 +11,7 @@
   return x;
 }
 
+#pragma clang diagnostic ignored "-Wmisleading-indentation"
 void foo() { int b=0; while (b==b); }
 
 // RUN: env CINDEXTEST_EDITING=1 CINDEXTEST_FAILONERROR=1 c-index-test -test-load-source-reparse 5 local \
Index: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -2525,19 +2525,18 @@
 
 // Find the most recent expression bound to the symbol in the current
 // context.
-  if (!ReferenceRegion) {
-if (const MemRegion *MR = C.getLocationRegionIfPostStore(N)) {
-  SVal Val = State->getSVal(MR);
-  if (Val.getAsLocSymbol() == Sym) {
-const VarRegion* VR = MR->getBaseRegion()->getAs();
-// Do not show local variables belonging to a function other than
-// where the error is reported.
-if (!VR ||
-(VR->getStackFrame() == LeakContext->getStackFrame()))
-  ReferenceRegion = MR;
-  }
+if (!ReferenceRegion) {
+  if (const MemRegion *MR = C.getLocationRegionIfPostStore(N)) {
+SVal Val = State->getSVal(MR);
+if (Val.getAsLocSymbol() == Sym) {
+  const VarRegion *VR = MR->getBaseRegion()->getAs();
+  // Do not show local variables belonging to a function other than
+  // where the error is reported.
+  if (!VR || (VR->getStackFrame() == LeakContext->getStackFrame()))
+ReferenceRegion = MR;
 }
   }
+}
 
 // Allocation node, is the last node in the current or parent context in
 // which the symbol was tracked.
Index: clang/lib/Parse/ParseStmt.cpp
===
--- clang/lib/Parse/ParseStmt.cpp
+++ clang/lib/Parse/ParseStmt.cpp
@@ -1191,6 +1191,55 @@
   return false;
 }
 
+namespace {
+
+enum MisleadingStatementKind { MSK_if, MSK_else, MSK_for, MSK_while };
+
+struct MisleadingIndentationChecker {
+  Parser 
+  SourceLocation StmtLoc;
+  SourceLocation PrevLoc;
+  unsigned NumDirectives;
+  MisleadingStatementKind Kind;
+  bool NeedsChecking;
+  bool HasBraces;
+  MisleadingIndentationChecker(Parser , MisleadingStatementKind K,
+   SourceLocation SL)
+  : P(P), StmtLoc(SL), PrevLoc(P.getCurToken().getLocation()),
+NumDirectives(P.getPreprocessor().getNumDirectives()), Kind(K),
+NeedsChecking(true), HasBraces(P.getCurToken().is(tok::l_brace)) {
+}
+  void Check(bool ShouldCheck) {
+NeedsChecking = false;
+Token Tok = P.getCurToken();
+if (!ShouldCheck || HasBraces ||
+NumDirectives != P.getPreprocessor().getNumDirectives() ||
+Tok.isOneOf(tok::semi, tok::r_brace) || Tok.isAnnotation() ||
+Tok.getLocation().isMacroID() || PrevLoc.isMacroID() ||
+

[PATCH] D70638: [Diagnostic] add a warning which warns about misleading indentation

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

Please land code fixes as NFC patch and then try to reland this patch.




Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2532
+if (Val.getAsLocSymbol() == Sym) {
+  const VarRegion *VR = MR->getBaseRegion()->getAs();
+  // Do not show local variables belonging to a function other than

Please land as separated NFCI commit.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70638/new/

https://reviews.llvm.org/D70638



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

[PATCH] D70638: [Diagnostic] add a warning which warns about misleading indentation

[Tyker via Phabricator via cfe-commits]

Tyker updated this revision to Diff 231534.
Tyker added a comment.

yeah i saw. this version of the patch builds all llvm subproject(not tested on 
llgo) without warnings. there is only one case in llvm's code in which this 
warning is more agressive thant GCC's.

  if (1)
i = 0;
  
  // comment
i = 1; //  clang + this patch warns through the comment wereas gcc doesn't.

i don't believe a warning in this case is too bad.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70638/new/

https://reviews.llvm.org/D70638

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Lex/Preprocessor.h
  clang/include/clang/Parse/Parser.h
  clang/lib/Parse/ParseStmt.cpp
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/test/Index/pragma-diag-reparse.c
  clang/test/Misc/warning-wall.c
  clang/test/Parser/warn-misleading-indentation.cpp
  clang/test/Preprocessor/pragma_diagnostic_sections.cpp

Index: clang/test/Preprocessor/pragma_diagnostic_sections.cpp
===
--- clang/test/Preprocessor/pragma_diagnostic_sections.cpp
+++ clang/test/Preprocessor/pragma_diagnostic_sections.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -Wall -Wunused-macros -Wunused-parameter -Wno-uninitialized -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wall -Wunused-macros -Wunused-parameter -Wno-uninitialized -Wno-misleading-indentation -verify %s
 
 // rdar://8365684
 struct S {
Index: clang/test/Parser/warn-misleading-indentation.cpp
===
--- /dev/null
+++ clang/test/Parser/warn-misleading-indentation.cpp
@@ -0,0 +1,210 @@
+// RUN: %clang_cc1 -x c -fsyntax-only -verify %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wmisleading-indentation -DWITH_WARN %s
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify -Wall -Wno-unused -DWITH_WARN -DCXX17 %s
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify -Wall -Wno-unused -Wno-misleading-indentation -DCXX17 %s
+
+#ifndef WITH_WARN
+// expected-no-diagnostics
+#endif
+
+void f0(int i) {
+  if (i)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = i + 1;
+int x = 0;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; declaration is not part of the previous 'if'}}
+#endif
+  return;
+#ifdef CXX17
+  if constexpr (false)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = 0;
+i += 1;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+#endif
+#endif
+}
+
+void f1(int i) {
+  for (;i;)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = i + 1;
+i *= 2;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'for'}}
+#endif
+  return;
+}
+
+void f2(int i) {
+  while (i)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = i + 1; i *= 2;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'while'}}
+#endif
+  return;
+}
+
+void f3(int i) {
+  if (i)
+i = i + 1;
+  else
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i *= 2;
+const int x = 0;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; declaration is not part of the previous 'else'}}
+#endif
+}
+
+#ifdef CXX17
+struct Range {
+  int *begin() {return nullptr;}
+  int *end() {return nullptr;}
+};
+#endif
+
+void f4(int i) {
+  if (i)
+  i *= 2;
+  return;
+  if (i)
+i *= 2;
+;
+  if (i)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i *= 2;
+typedef int Int;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; declaration is not part of the previous 'if'}}
+#endif
+#ifdef CXX17
+  Range R;
+  for (auto e : R)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i *= 2;
+using Int2 = int;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; declaration is not part of the previous 'for'}}
+#endif
+#endif
+}
+
+int bar(void);
+
+int foo(int* dst)
+{   
+if (dst)
+   return
+bar();
+  if (dst)
+dst = dst + \
+bar();
+  return 0;
+}
+
+// No diagnostics from GCC on this
+void g(int i) {
+  if (1)
+i = 2;
+  else
+ if (i == 3)
+i = 4;
+i = 5;
+}
+
+// Or this
+#define TEST i = 5
+void g0(int i) {
+  if (1)
+i = 2;
+  else
+i = 5;
+TEST;
+}
+
+void g1(int i) {
+  if (1)
+i = 2;
+  else
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+if (i == 3)
+i = 4;
+i = 5;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'else'}}
+#endif
+}
+
+void g2(int i) {
+  if (1)
+i = 2;
+  else
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+if (i == 3)
+{i = 4;}
+i = 5;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not 

[PATCH] D70638: [Diagnostic] add a warning which warns about misleading indentation

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 reopened this revision.
xbolva00 added a subscriber: tstellar.
xbolva00 added a comment.
This revision is now accepted and ready to land.

@tstellar reverted this change.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70638/new/

https://reviews.llvm.org/D70638



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

[PATCH] D70638: [Diagnostic] add a warning which warns about misleading indentation

[Tyker via Phabricator via cfe-commits]

Tyker added a comment.

Thanks for the review


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70638/new/

https://reviews.llvm.org/D70638



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

[PATCH] D70638: [Diagnostic] add a warning which warns about misleading indentation

[Tyker via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG7b86188b50bf: [Diagnostic] add a warning which warns about 
misleading indentation (authored by Tyker).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70638/new/

https://reviews.llvm.org/D70638

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Parse/Parser.h
  clang/lib/Parse/ParseStmt.cpp
  clang/test/Index/pragma-diag-reparse.c
  clang/test/Misc/warning-wall.c
  clang/test/Preprocessor/pragma_diagnostic_sections.cpp
  clang/test/SemaCXX/warn-misleading-indentation.cpp

Index: clang/test/SemaCXX/warn-misleading-indentation.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-misleading-indentation.cpp
@@ -0,0 +1,184 @@
+// RUN: %clang_cc1 -x c -fsyntax-only -verify %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wmisleading-indentation -DWITH_WARN %s
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify -Wall -Wno-unused -DWITH_WARN -DCXX17 %s
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify -Wall -Wno-unused -Wno-misleading-indentation -DCXX17 %s
+
+#ifndef WITH_WARN
+// expected-no-diagnostics
+#endif
+
+void f0(int i) {
+  if (i)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = i + 1;
+int x = 0;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; declaration is not part of the previous 'if'}}
+#endif
+  return;
+#ifdef CXX17
+  if constexpr (false)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = 0;
+i += 1;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+#endif
+#endif
+}
+
+void f1(int i) {
+  for (;i;)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = i + 1;
+i *= 2;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'for'}}
+#endif
+  return;
+}
+
+void f2(int i) {
+  while (i)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = i + 1; i *= 2;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'while'}}
+#endif
+  return;
+}
+
+void f3(int i) {
+  if (i)
+i = i + 1;
+  else
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i *= 2;
+const int x = 0;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; declaration is not part of the previous 'else'}}
+#endif
+}
+
+#ifdef CXX17
+struct Range {
+  int *begin() {return nullptr;}
+  int *end() {return nullptr;}
+};
+#endif
+
+void f4(int i) {
+  if (i)
+  i *= 2;
+  return;
+  if (i)
+i *= 2;
+;
+  if (i)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i *= 2;
+typedef int Int;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; declaration is not part of the previous 'if'}}
+#endif
+#ifdef CXX17
+  Range R;
+  for (auto e : R)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i *= 2;
+using Int2 = int;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; declaration is not part of the previous 'for'}}
+#endif
+#endif
+}
+
+int bar(void);
+
+int foo(int* dst)
+{   
+if (dst)
+   return
+bar();
+  if (dst)
+dst = dst + \
+bar();
+  return 0;
+}
+
+// No diagnostics from GCC on this
+void g(int i) {
+  if (1)
+i = 2;
+  else
+ if (i == 3)
+i = 4;
+i = 5;
+}
+
+// Or this
+#define TEST i = 5
+void g0(int i) {
+  if (1)
+i = 2;
+  else
+i = 5;
+TEST;
+}
+
+void g1(int i) {
+  if (1)
+i = 2;
+  else
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+if (i == 3)
+i = 4;
+i = 5;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'else'}}
+#endif
+}
+
+void g2(int i) {
+  if (1)
+i = 2;
+  else
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+if (i == 3)
+{i = 4;}
+i = 5;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'else'}}
+#endif
+}
+
+void g6(int i) {
+if (1)
+  if (i == 3)
+#ifdef WITH_WARN
+// expected-note@-2 {{here}}
+#endif
+i = 4;
+i = 5;
+#ifdef WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+#endif
+}
Index: clang/test/Preprocessor/pragma_diagnostic_sections.cpp
===
--- clang/test/Preprocessor/pragma_diagnostic_sections.cpp
+++ clang/test/Preprocessor/pragma_diagnostic_sections.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -Wall -Wunused-macros -Wunused-parameter -Wno-uninitialized -verify %s
+// RUN: %clang_cc1 -fsyntax-only