Page MenuHomePhabricator

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

Authored by guiguiiiiiiii on Jun 16 2018, 5:21 PM.

Details

Reviewers
klimek
djasper
Summary

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 aaaa = 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)

Diff Detail

Repository
rC Clang

Event Timeline

guiguiiiiiiii created this revision.Jun 16 2018, 5:21 PM
klimek added inline comments.Jun 17 2018, 11:10 PM
lib/Format/WhitespaceManager.cpp
678

Why is this not just if (FirstTabWidth <= Spaces) then?

unittests/Format/FormatTest.cpp
9372

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).

guiguiiiiiiii added inline comments.Jun 19 2018, 12:14 PM
lib/Format/WhitespaceManager.cpp
678

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 aaaa = 42;

would become (following what's written in the documentation)

int a    = 42;
int aa   = 42;
int aaa  = 42;
int aaaa = 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 aaaa = 42; // int aaaa = 42;
unittests/Format/FormatTest.cpp
9372

I guess a new test would be better since the unit tests are really lacking toward using tabs.

klimek added inline comments.Jun 21 2018, 6:56 AM
lib/Format/WhitespaceManager.cpp
678

The intent of the Always setting was to mirror what the Linux kernel wants, so whatever that is, we should probably follow it :)

guiguiiiiiiii abandoned this revision.Mar 3 2019, 4:41 AM

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.

Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2019, 4:41 AM