[PATCH] D48259: [clang-format] Fix bug with UT_Always when there is less than one full tab

[Guillaume Reymond via Phabricator via cfe-commits]

guigu created this revision.
guigu added reviewers: klimek, djasper.
guigu added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

There's a bug in the `WhitespaceManager` that prevents correct alignment when 
using tab.
This patch fix a test condition that was triggering the bug and add missing 
unit tests that would have failed.

The issue can be seen by invoking clang-format on this simple snippet with 
`-style="{BasedOnStyle: llvm, UseTab: Always, AlignConsecutiveAssignments: 
true, AlignConsecutiveDeclarations: true, TabWidth: 4}"`

  int a = 42;
  int aa = 42;
  int aaa = 42;
  int  = 42;

Displaying the result in a editor with a tabsize of 4, the first = won't be 
aligned correctly.

The logic in `WhitespaceManager::appendIndentText` treats the first tab as a 
special case, which is what must be done in order to handle tabstop correctly.
However, handling the first tab is done conditionally using the first tab width.
The proposed fix is to handle the first tab independently of its size as the 
code that follow this test...

  Text.append(Spaces / Style.TabWidth, '\t');

...will only work if the `'\t'` we're adding are positionned on a tabstop (so 
`'\t'` is equal to **TabWidth** in the output, otherwise it will be between 1 
and **TabWidth** - 1)


Repository:
  rC Clang

https://reviews.llvm.org/D48259

Files:
  lib/Format/WhitespaceManager.cpp
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -9367,6 +9367,18 @@
"int oneTwoThree = 123;\n"
"int oneTwo = 12;",
Alignment));
+  // Test with use of tabs for alignment
+  FormatStyle::UseTabStyle OldTabStyle = Alignment.UseTab;
+  unsigned OldTabWidth = Alignment.TabWidth;
+  Alignment.UseTab = FormatStyle::UT_Always;
+  Alignment.TabWidth = 4;
+  verifyFormat("int a\t = 42;\n"
+   "int aa   = 42;\n"
+   "int aaa  = 42;\n"
+   "int  = 42;",
+   Alignment);
+  Alignment.UseTab = OldTabStyle;
+  Alignment.TabWidth = OldTabWidth;
   Alignment.AlignEscapedNewlines = FormatStyle::ENAS_DontAlign;
   verifyFormat("#define A \\\n"
"  int    = 12; \\\n"
@@ -9542,6 +9554,18 @@
"unsigned oneTwoThree = 123;\n"
"int oneTwo = 12;",
Alignment));
+  // Test with use of tabs for alignment
+  FormatStyle::UseTabStyle OldTabStyle = Alignment.UseTab;
+  unsigned OldTabWidth = Alignment.TabWidth;
+  Alignment.UseTab = FormatStyle::UT_Always;
+  Alignment.TabWidth = 4;
+  verifyFormat("int\t   a = 42;\n"
+   "float  b = 42;\n"
+   "char   c = 42;\n"
+   "double d = 42;",
+   Alignment);
+  Alignment.UseTab = OldTabStyle;
+  Alignment.TabWidth = OldTabWidth;
   // Function prototype alignment
   verifyFormat("inta();\n"
"double b();",
Index: lib/Format/WhitespaceManager.cpp
===
--- lib/Format/WhitespaceManager.cpp
+++ lib/Format/WhitespaceManager.cpp
@@ -675,7 +675,7 @@
 unsigned FirstTabWidth =
 Style.TabWidth - WhitespaceStartColumn % Style.TabWidth;
 // Indent with tabs only when there's at least one full tab.
-if (FirstTabWidth + Style.TabWidth <= Spaces) {
+if (Style.TabWidth <= Spaces) {
   Spaces -= FirstTabWidth;
   Text.append("\t");
 }


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -9367,6 +9367,18 @@
"int oneTwoThree = 123;\n"
"int oneTwo = 12;",
Alignment));
+  // Test with use of tabs for alignment
+  FormatStyle::UseTabStyle OldTabStyle = Alignment.UseTab;
+  unsigned OldTabWidth = Alignment.TabWidth;
+  Alignment.UseTab = FormatStyle::UT_Always;
+  Alignment.TabWidth = 4;
+  verifyFormat("int a\t = 42;\n"
+   "int aa   = 42;\n"
+   "int aaa  = 42;\n"
+   "int  = 42;",
+   Alignment);
+  Alignment.UseTab = OldTabStyle;
+  Alignment.TabWidth = OldTabWidth;
   Alignment.AlignEscapedNewlines = FormatStyle::ENAS_DontAlign;
   verifyFormat("#define A \\\n"
"  int    = 12; \\\n"
@@ -9542,6 +9554,18 @@
"unsigned oneTwoThree = 123;\n"
"int oneTwo = 12;",
Alignment));
+  // Test with use of tabs for alignment
+  FormatStyle::UseTabStyle OldTabStyle = Alignment.UseTab;
+  unsigned OldTabWidth = Alignment.TabWidth;
+  Alignment.UseTab = FormatStyle::UT_Always;
+  Alignment.TabWidth = 4;
+  verifyFormat("int\t   a = 42;\n"
+   "float  b = 42;\n"
+   "char

[PATCH] D48259: [clang-format] Fix bug with UT_Always when there is less than one full tab

[Guillaume Reymond via Phabricator via cfe-commits]

guigu added inline comments.



Comment at: lib/Format/WhitespaceManager.cpp:678
 // Indent with tabs only when there's at least one full tab.
-if (FirstTabWidth + Style.TabWidth <= Spaces) {
+if (Style.TabWidth <= Spaces) {
   Spaces -= FirstTabWidth;

klimek wrote:
> Why is this not just if (FirstTabWidth <= Spaces) then?
Actually, that was my first way of fixing it. 

```
if (FirstTabWidth <= Spaces && Spaces > 1) // to avoid converting a single 
space to a tab
```

Doing so will produce a correct output but introduce what I thought could be an 
unwanted change : whitespace less than **TabWidth** might get converted to a 
tab.
The comment above the test seems to indicate that this behavior was not 
originally wanted, that's why I changed my mind. 

But after thinking it over, I think my fix attempt also introduce an unwanted 
change. I misinterpreted the original goal of this option.
I went back to the official documentation and it says

> Use tabs whenever we need to fill whitespace that spans at least from one tab 
> stop to the next one

This is clearly not what this code is doing. I can land another patch for a 
more appropriate fix but I wonder if this option should stay like this. Aren't 
people expecting the formatting to use as much tab as possible with UT_Always ?

Unless I'm mistaken, the following example (**TabWidth** of 4)


```
int a = 42;
int aa = 42;
int aaa = 42;
int  = 42;
```
would become (following what's written in the documentation)

```
int a= 42;
int aa   = 42;
int aaa  = 42;
int  = 42; 
// only spaces since there's never enough whitespace to span a full tab stop
```
And this is what I would expect:

```
int a= 42; // int a\t = 42;
int aa   = 42; // int aa\t = 42;
int aaa  = 42; // int aaa\t = 42;
int  = 42; // int  = 42;
```



Comment at: unittests/Format/FormatTest.cpp:9372
+  FormatStyle::UseTabStyle OldTabStyle = Alignment.UseTab;
+  unsigned OldTabWidth = Alignment.TabWidth;
+  Alignment.UseTab = FormatStyle::UT_Always;

klimek wrote:
> Instead of doing the save/restore dance, just put it at the end of a test or 
> create a new test (or alternatively create a new style as copy and then 
> change the settings on that).
I guess a new test would be better since the unit tests are really lacking 
toward using tabs.


Repository:
  rC Clang

https://reviews.llvm.org/D48259



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

[PATCH] D48259: [clang-format] Fix bug with UT_Always when there is less than one full tab

[Guillaume Reymond via Phabricator via cfe-commits]

guigu abandoned this revision.
guigu added a comment.
Herald added a project: clang.

Forgot about this one.
While merging, noticed that it's fixed by https://reviews.llvm.org/D57655. The 
integrated fix is better as it handle the unlikely but possible tabwidth of 1.
Closing.


Repository:
  rC Clang

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

https://reviews.llvm.org/D48259



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