[PATCH] D71037: [Diagnostic] Add ftabstop to -Wmisleading-indentation

[Tyker via Phabricator via cfe-commits]

Tyker added a comment.

In D71037#1871089 , @efriedma wrote:

> I just ran into this warning, and I think there's a bit of a discoverability 
> problem related to the width of tabs and -ftabstop.  If you have mixed tabs 
> and spaces, and you've correctly specified the tab stop width with -ftabstop, 
> everything works fine.  If you haven't specified -ftabstop, you get a 
> warning, and the issue isn't obvious.  Depending on your editor settings, the 
> code might look fine at first glance, and the warning text doesn't mention 
> -ftabstop at all.  Maybe there's something that could be improved here, if 
> we're printing a warning for two lines with mismatched indentation styles?


i agree that this is not obvious. i added a patch to improve this 
https://reviews.llvm.org/D75009.


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

https://reviews.llvm.org/D71037



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

[PATCH] D71037: [Diagnostic] Add ftabstop to -Wmisleading-indentation

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

I just ran into this warning, and I think there's a bit of a discoverability 
problem related to the width of tabs and -ftabstop.  If you have mixed tabs and 
spaces, and you've correctly specified the tab stop width with -ftabstop, 
everything works fine.  If you haven't specified -ftabstop, you get a warning, 
and the issue isn't obvious.  Depending on your editor settings, the code might 
look fine at first glance, and the warning text doesn't mention -ftabstop at 
all.  Maybe there's something that could be improved here, if we're printing a 
warning for two lines with mismatched indentation styles?


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

https://reviews.llvm.org/D71037



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

[PATCH] D71037: [Diagnostic] Add ftabstop to -Wmisleading-indentation

[Tyker via Phabricator via cfe-commits]

Tyker closed this revision.
Tyker added a comment.

In D71037#1803704 , @mstorsjo wrote:

> In D71037#1803574 , @Tyker wrote:
>
> > In D71037#1803361 , @xbolva00 
> > wrote:
> >
> > > Can you add a test case for that crash? Otherwise OK.
> >
> >
> > as i said. the bug can only occur for one line files. so we can't have a 
> > run line and a the crashing line. so i wasn't able to make a test for it.
>
>
> Is it possible to have the `RUN` bit at the end of the line in a comment, 
> after the test code itself?


it is possible.

i committed the fix and the tests.


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

https://reviews.llvm.org/D71037



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

[PATCH] D71037: [Diagnostic] Add ftabstop to -Wmisleading-indentation

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D71037#1803574 , @Tyker wrote:

> In D71037#1803361 , @xbolva00 wrote:
>
> > Can you add a test case for that crash? Otherwise OK.
>
>
> as i said. the bug can only occur for one line files. so we can't have a run 
> line and a the crashing line. so i wasn't able to make a test for it.


Is it possible to have the `RUN` bit at the end of the line in a comment, after 
the test code itself?


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

https://reviews.llvm.org/D71037



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

[PATCH] D71037: [Diagnostic] Add ftabstop to -Wmisleading-indentation

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

xbolva00 added a comment.

Ah, true.


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

https://reviews.llvm.org/D71037



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

[PATCH] D71037: [Diagnostic] Add ftabstop to -Wmisleading-indentation

[Tyker via Phabricator via cfe-commits]

Tyker added a comment.

In D71037#1803361 , @xbolva00 wrote:

> Can you add a test case for that crash? Otherwise OK.


as i said. the bug can only occur for one line files. so we can't have a run 
line and a the crashing line. so i wasn't able to make a test for it.


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

https://reviews.llvm.org/D71037



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

[PATCH] D71037: [Diagnostic] Add ftabstop to -Wmisleading-indentation

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

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

Can you add a test case for that crash? Otherwise OK.


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

https://reviews.llvm.org/D71037



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

[PATCH] D71037: [Diagnostic] Add ftabstop to -Wmisleading-indentation

[Tyker via Phabricator via cfe-commits]

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

This update should fixes the issue.
the assert was incorrect. because file offsets are 0-based where as column 
numbers are 1-based.


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

https://reviews.llvm.org/D71037

Files:
  clang/lib/Parse/ParseStmt.cpp
  clang/test/Parser/warn-misleading-indentation.cpp

Index: clang/test/Parser/warn-misleading-indentation.cpp
===
--- clang/test/Parser/warn-misleading-indentation.cpp
+++ clang/test/Parser/warn-misleading-indentation.cpp
@@ -1,7 +1,9 @@
 // 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
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wmisleading-indentation -DWITH_WARN -ftabstop 8 -DTAB_SIZE=8 %s
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify -Wall -Wno-unused -DWITH_WARN  -ftabstop 4 -DTAB_SIZE=4 -DCXX17 %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wall -Wno-unused -DWITH_WARN -ftabstop 1 -DTAB_SIZE=1 %s
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify -Wall -Wno-unused -Wmisleading-indentation -DCXX17 -DWITH_WARN -ftabstop 2 -DTAB_SIZE=2 %s
 
 #ifndef WITH_WARN
 // expected-no-diagnostics
@@ -225,3 +227,81 @@
 // expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
 #endif
 }
+int a4()
+{
+	if (0)
+		return 1;
+ 	return 0;
+#if (TAB_SIZE == 1)
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+// expected-note@-5 {{here}}
+#endif 
+}
+
+int a5()
+{
+	if (0)
+		return 1;
+		return 0;
+#if WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+// expected-note@-5 {{here}}
+#endif
+}
+
+int a6()
+{
+	if (0)
+		return 1;
+  		return 0;
+#if (TAB_SIZE == 8)
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+// expected-note@-5 {{here}}
+#endif
+}
+
+#define FOO \
+ goto fail
+
+int main(int argc, char* argv[]) {
+  if (5 != 0)
+goto fail;
+  else
+goto fail;
+
+  if (1) {
+if (1)
+  goto fail;
+else if (1)
+  goto fail;
+else if (1)
+  goto fail;
+else
+  goto fail;
+  } else if (1) {
+if (1)
+  goto fail;
+  }
+
+  if (1) {
+if (1)
+  goto fail;
+  } else if (1)
+goto fail;
+
+
+  if (1) goto fail; goto fail;
+
+if (0)
+goto fail;
+
+goto fail;
+
+if (0)
+FOO;
+
+goto fail;
+
+fail:;
+}
+
Index: clang/lib/Parse/ParseStmt.cpp
===
--- clang/lib/Parse/ParseStmt.cpp
+++ clang/lib/Parse/ParseStmt.cpp
@@ -1214,6 +1214,42 @@
 if (Kind == MSK_else && !ShouldSkip)
   P.MisleadingIndentationElseLoc = SL;
   }
+
+  /// Compute the column number will aligning tabs on TabStop (-ftabstop), this
+  /// gives the visual indentation of the SourceLocation.
+  static unsigned getVisualIndentation(SourceManager , SourceLocation Loc) {
+unsigned TabStop = SM.getDiagnostics().getDiagnosticOptions().TabStop;
+
+unsigned ColNo = SM.getSpellingColumnNumber(Loc);
+if (ColNo == 0 || TabStop == 1)
+  return ColNo;
+
+std::pair FIDAndOffset = SM.getDecomposedLoc(Loc);
+
+bool Invalid;
+StringRef BufData = SM.getBufferData(FIDAndOffset.first, );
+if (Invalid)
+  return 0;
+
+const char *EndPos = BufData.data() + FIDAndOffset.second;
+// FileOffset are 0-based and Column numbers are 1-based
+assert(FIDAndOffset.second + 1 >= ColNo &&
+   "Column number smaller than file offset?");
+
+unsigned VisualColumn = 0; // Stored as 0-based column, here.
+// Loop from beginning of line up to Loc's file position, counting columns,
+// expanding tabs.
+for (const char *CurPos = EndPos - (ColNo - 1); CurPos != EndPos;
+ ++CurPos) {
+  if (*CurPos == '\t')
+// Advance visual column to next tabstop.
+VisualColumn += (TabStop - VisualColumn % TabStop);
+  else
+VisualColumn++;
+}
+return VisualColumn + 1;
+  }
+
   void Check() {
 Token Tok = P.getCurToken();
 if (P.getActions().getDiagnostics().isIgnored(
@@ -1230,9 +1266,9 @@
   P.MisleadingIndentationElseLoc = SourceLocation();
 
 SourceManager  = P.getPreprocessor().getSourceManager();
-unsigned PrevColNum = SM.getSpellingColumnNumber(PrevLoc);
-unsigned CurColNum = SM.getSpellingColumnNumber(Tok.getLocation());
-unsigned StmtColNum = SM.getSpellingColumnNumber(StmtLoc);
+unsigned PrevColNum = getVisualIndentation(SM, PrevLoc);
+unsigned CurColNum = getVisualIndentation(SM, Tok.getLocation());
+unsigned 

[PATCH] D71037: [Diagnostic] Add ftabstop to -Wmisleading-indentation

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D71037#1799792 , @mstorsjo wrote:

> This broke my builds.


I pushed a revert for this for now, to unbreak things.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71037



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

[PATCH] D71037: [Diagnostic] Add ftabstop to -Wmisleading-indentation

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

This broke my builds. A configure test that tries to compile `int main (void) { 
for( int i = 0; i < 9; i++ ); return 0; }` (a single line source file), with 
`-Wall` enabled, now triggers failed asserts:

  clang-10: ../tools/clang/lib/Parse/ParseStmt.cpp:1236: static unsigned int 
{anonymous}::MisleadingIndentationChecker::getVisualIndentation(clang::SourceManager&,
 clang::SourceLocation): Assertion `FIDAndOffset.second > ColNo && "Column 
number smaller than file offset?"' failed.
  
  
  
  1.  oneline.c:1:49: current parser token 'return'
  2.  oneline.c:1:17: parsing function body 'main'
  3.  oneline.c:1:17: in compound statement ('{}')


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71037



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

[PATCH] D71037: [Diagnostic] Add ftabstop to -Wmisleading-indentation

[Tyker via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGb47b35ff51b3: [Diagnostic] Add ftabstop to 
-Wmisleading-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/D71037/new/

https://reviews.llvm.org/D71037

Files:
  clang/lib/Parse/ParseStmt.cpp
  clang/test/Parser/warn-misleading-indentation.cpp

Index: clang/test/Parser/warn-misleading-indentation.cpp
===
--- clang/test/Parser/warn-misleading-indentation.cpp
+++ clang/test/Parser/warn-misleading-indentation.cpp
@@ -1,7 +1,9 @@
 // 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
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wmisleading-indentation -DWITH_WARN -ftabstop 8 -DTAB_SIZE=8 %s
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify -Wall -Wno-unused -DWITH_WARN  -ftabstop 4 -DTAB_SIZE=4 -DCXX17 %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wall -Wno-unused -DWITH_WARN -ftabstop 1 -DTAB_SIZE=1 %s
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify -Wall -Wno-unused -Wmisleading-indentation -DCXX17 -DWITH_WARN -ftabstop 2 -DTAB_SIZE=2 %s
 
 #ifndef WITH_WARN
 // expected-no-diagnostics
@@ -225,3 +227,80 @@
 // expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
 #endif
 }
+int a4()
+{
+	if (0)
+		return 1;
+ 	return 0;
+#if (TAB_SIZE == 1)
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+// expected-note@-5 {{here}}
+#endif 
+}
+
+int a5()
+{
+	if (0)
+		return 1;
+		return 0;
+#if WITH_WARN
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+// expected-note@-5 {{here}}
+#endif
+}
+
+int a6()
+{
+	if (0)
+		return 1;
+  		return 0;
+#if (TAB_SIZE == 8)
+// expected-warning@-2 {{misleading indentation; statement is not part of the previous 'if'}}
+// expected-note@-5 {{here}}
+#endif
+}
+
+#define FOO \
+ goto fail
+
+int main(int argc, char* argv[]) {
+  if (5 != 0)
+goto fail;
+  else
+goto fail;
+
+  if (1) {
+if (1)
+  goto fail;
+else if (1)
+  goto fail;
+else if (1)
+  goto fail;
+else
+  goto fail;
+  } else if (1) {
+if (1)
+  goto fail;
+  }
+
+  if (1) {
+if (1)
+  goto fail;
+  } else if (1)
+goto fail;
+
+
+  if (1) goto fail; goto fail;
+
+if (0)
+goto fail;
+
+goto fail;
+
+if (0)
+FOO;
+
+goto fail;
+
+fail:;
+}
Index: clang/lib/Parse/ParseStmt.cpp
===
--- clang/lib/Parse/ParseStmt.cpp
+++ clang/lib/Parse/ParseStmt.cpp
@@ -1214,6 +1214,41 @@
 if (Kind == MSK_else && !ShouldSkip)
   P.MisleadingIndentationElseLoc = SL;
   }
+
+  /// Compute the column number will aligning tabs on TabStop (-ftabstop), this
+  /// gives the visual indentation of the SourceLocation.
+  static unsigned getVisualIndentation(SourceManager , SourceLocation Loc) {
+unsigned TabStop = SM.getDiagnostics().getDiagnosticOptions().TabStop;
+
+unsigned ColNo = SM.getSpellingColumnNumber(Loc);
+if (ColNo == 0 || TabStop == 1)
+  return ColNo;
+
+std::pair FIDAndOffset = SM.getDecomposedLoc(Loc);
+
+bool Invalid;
+StringRef BufData = SM.getBufferData(FIDAndOffset.first, );
+if (Invalid)
+  return 0;
+
+const char *EndPos = BufData.data() + FIDAndOffset.second;
+assert(FIDAndOffset.second > ColNo &&
+   "Column number smaller than file offset?");
+
+unsigned VisualColumn = 0; // Stored as 0-based column, here.
+// Loop from beginning of line up to Loc's file position, counting columns,
+// expanding tabs.
+for (const char *CurPos = EndPos - (ColNo - 1); CurPos != EndPos;
+ ++CurPos) {
+  if (*CurPos == '\t')
+// Advance visual column to next tabstop.
+VisualColumn += (TabStop - VisualColumn % TabStop);
+  else
+VisualColumn++;
+}
+return VisualColumn + 1;
+  }
+
   void Check() {
 Token Tok = P.getCurToken();
 if (P.getActions().getDiagnostics().isIgnored(
@@ -1230,9 +1265,9 @@
   P.MisleadingIndentationElseLoc = SourceLocation();
 
 SourceManager  = P.getPreprocessor().getSourceManager();
-unsigned PrevColNum = SM.getSpellingColumnNumber(PrevLoc);
-unsigned CurColNum = SM.getSpellingColumnNumber(Tok.getLocation());
-unsigned StmtColNum = SM.getSpellingColumnNumber(StmtLoc);
+unsigned PrevColNum = getVisualIndentation(SM, PrevLoc);
+unsigned CurColNum = getVisualIndentation(SM,