clang-format with UseTab: Always sometimes doesn't insert the right amount of tabs.

[hyklv]

Summary

Hi,

Trailing comments are not always aligned properly when UseTab is set to Always.

Consider:

int a;	      // x
int bbbbbbbb; // x

With .clang-format:

---
Language:        Cpp
BasedOnStyle: LLVM
UseTab: Always
...

The trailing comments of this code block should be aligned, but aren't

To align the first trailing comment it needs to insert 8 spaces. This should be one tab plus six spaces. It skips the logic of the first partial tab in FirstTabWidth (=2) + Style.TabWidth (=8) <= Spaces (=8) and only inserts one tab. Proposed fix and test is attached.

[lebedev.ri]

Test?
Is this a fix, or a new formatting style?

WhitespaceManager.cpp

3–6 ↗

(On Diff #184956)

That is incorrect.

[MyDeveloperDay]

The reviewers set a high barrier of entry, you need to at least start by adding some unit tests (or someone will break your code in the future), if you think it was a bug it might be worth logging that in bugzilla too!

[hyklv]

I removed accidental changes in the copyright notice, updated the code to insert one space instead of one tab. Added a test case for the alignment.

[hyklv]

In D57655#1382437, @lebedev.ri wrote:

Test?
Is this a fix, or a new formatting style?

a fix.

[hyklv]

In D57655#1382855, @MyDeveloperDay wrote:

The reviewers set a high barrier of entry, you need to at least start by adding some unit tests (or someone will break your code in the future), if you think it was a bug it might be worth logging that in bugzilla too!

thanks for pointing me in the right direction! i added a test.

[alexfh]

Seems reasonable. LG with a couple of nits. Please let me know if you need to commit this for you.

D:/Format/llvm/tools/clang/lib/Format/WhitespaceManager.cpp

684

nit: I'd just break here and remove the else.

685

nit: "Align to the next tab."

[hyklv]

Updated comment and added break;

[hyklv]

In D57655#1395924, @alexfh wrote:

Seems reasonable. LG with a couple of nits. Please let me know if you need to commit this for you.

cool. could you commit the change for me?

D:/Format/llvm/tools/clang/lib/Format/WhitespaceManager.cpp

void WhitespaceManager::appendIndentText(std::string &Text,
                                          unsigned WhitespaceStartColumn) {
   switch (Style.UseTab) {
   case FormatStyle::UT_Never:
     Text.append(Spaces, ' ');
     break;
   case FormatStyle::UT_Always: {
     unsigned FirstTabWidth =
         Style.TabWidth - WhitespaceStartColumn % Style.TabWidth;
-    // Indent with tabs only when there's at least one full tab.
-    if (FirstTabWidth + Style.TabWidth <= Spaces) {
-      Spaces -= FirstTabWidth;
-      Text.append("\t");
-    }
+    // Insert only spaces when we want to end up before the next tab.
+    if (Spaces < FirstTabWidth || Spaces == 1) {
+      Text.append(Spaces, ' ');
+      break;
+    } 
+    // Align to the next tab.
+
     Text.append(Spaces / Style.TabWidth, '\t');
     Text.append(Spaces % Style.TabWidth, ' ');
     break;
   }
   case FormatStyle::UT_ForIndentation:
     if (WhitespaceStartColumn == 0) {
       unsigned Indentation = IndentLevel * Style.IndentWidth;
       // This happens, e.g. when a line in a block comment is indented less than

D:/Format/llvm/tools/clang/unittests/Format/FormatTest.cpp

verifyFormat("class X {\n"
                Tab);
   verifyFormat("#define A                        \\\n"
                "\tvoid f() {               \\\n"
                "\t\tsomeFunction(    \\\n"
                "\t\t    parameter1,  \\\n"
                "\t\t    parameter2); \\\n"
                "\t}",
                Tab);
+  verifyFormat("int a;\t      // x\n"
+               "int bbbbbbbb; // x\n",
+               Tab);
 
   Tab.TabWidth = 4;
   Tab.IndentWidth = 8;
   verifyFormat("class TabWidth4Indent8 {\n"
                "\t\tvoid f() {\n"
                "\t\t\t\tsomeFunction(parameter1,\n"
                "\t\t\t\t\t\t\t parameter2);\n"
                "\t\t}\n"