Page MenuHomePhabricator

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

Authored by hyklv on Feb 3 2019, 8:50 AM.

Details

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.

Diff Detail

Repository
rC Clang

Event Timeline

hyklv created this revision.Feb 3 2019, 8:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 3 2019, 8:50 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
lebedev.ri requested changes to this revision.Feb 3 2019, 9:07 AM
lebedev.ri added a subscriber: lebedev.ri.

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

WhitespaceManager.cpp
3–6 ↗(On Diff #184956)

That is incorrect.

This revision now requires changes to proceed.Feb 3 2019, 9:07 AM
MyDeveloperDay added a subscriber: MyDeveloperDay.EditedFeb 4 2019, 2:24 AM

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 edited the summary of this revision. (Show Details)Feb 11 2019, 11:02 AM
hyklv updated this revision to Diff 186315.Feb 11 2019, 1:08 PM
hyklv marked an inline comment as done.

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 added a comment.Feb 11 2019, 1:09 PM

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

a fix.

hyklv added a comment.Feb 11 2019, 1:10 PM

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.

hyklv edited the summary of this revision. (Show Details)Feb 11 2019, 1:17 PM
lebedev.ri resigned from this revision.Feb 13 2019, 12:15 AM
alexfh accepted this revision.Feb 13 2019, 2:00 AM

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
685 ↗(On Diff #186315)

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

686 ↗(On Diff #186315)

nit: "Align to the next tab."

This revision is now accepted and ready to land.Feb 13 2019, 2:00 AM
hyklv updated this revision to Diff 186629.Feb 13 2019, 5:27 AM
hyklv marked 2 inline comments as done.

Updated comment and added break;

hyklv added a comment.Feb 13 2019, 5:29 AM

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?

This revision was automatically updated to reflect the committed changes.