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

[Hylke Kleve via Phabricator via cfe-commits]

hyklv added a comment.

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?


Repository:
  rC Clang

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

https://reviews.llvm.org/D57655



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

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

[Hylke Kleve via Phabricator via cfe-commits]

hyklv updated this revision to Diff 186629.
hyklv marked 2 inline comments as done.
hyklv added a comment.

Updated comment and added break;


Repository:
  rC Clang

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

https://reviews.llvm.org/D57655

Files:
  D:/Format/llvm/tools/clang/lib/Format/WhitespaceManager.cpp
  D:/Format/llvm/tools/clang/unittests/Format/FormatTest.cpp


Index: D:/Format/llvm/tools/clang/unittests/Format/FormatTest.cpp
===
--- D:/Format/llvm/tools/clang/unittests/Format/FormatTest.cpp
+++ D:/Format/llvm/tools/clang/unittests/Format/FormatTest.cpp
@@ -8739,6 +8739,9 @@
"\t\tparameter2); \\\n"
"\t}",
Tab);
+  verifyFormat("int a;\t  // x\n"
+   "int ; // x\n",
+   Tab);
 
   Tab.TabWidth = 4;
   Tab.IndentWidth = 8;
Index: D:/Format/llvm/tools/clang/lib/Format/WhitespaceManager.cpp
===
--- D:/Format/llvm/tools/clang/lib/Format/WhitespaceManager.cpp
+++ D:/Format/llvm/tools/clang/lib/Format/WhitespaceManager.cpp
@@ -679,11 +679,15 @@
   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.
+Spaces -= FirstTabWidth;
+Text.append("\t");
+
 Text.append(Spaces / Style.TabWidth, '\t');
 Text.append(Spaces % Style.TabWidth, ' ');
 break;


Index: D:/Format/llvm/tools/clang/unittests/Format/FormatTest.cpp
===
--- D:/Format/llvm/tools/clang/unittests/Format/FormatTest.cpp
+++ D:/Format/llvm/tools/clang/unittests/Format/FormatTest.cpp
@@ -8739,6 +8739,9 @@
"\t\tparameter2); \\\n"
"\t}",
Tab);
+  verifyFormat("int a;\t  // x\n"
+   "int ; // x\n",
+   Tab);
 
   Tab.TabWidth = 4;
   Tab.IndentWidth = 8;
Index: D:/Format/llvm/tools/clang/lib/Format/WhitespaceManager.cpp
===
--- D:/Format/llvm/tools/clang/lib/Format/WhitespaceManager.cpp
+++ D:/Format/llvm/tools/clang/lib/Format/WhitespaceManager.cpp
@@ -679,11 +679,15 @@
   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.
+Spaces -= FirstTabWidth;
+Text.append("\t");
+
 Text.append(Spaces / Style.TabWidth, '\t');
 Text.append(Spaces % Style.TabWidth, ' ');
 break;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Hylke Kleve via Phabricator via cfe-commits]

hyklv added a comment.

In D57655#1382437 , @lebedev.ri wrote:

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


a fix.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57655



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

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

[Hylke Kleve via Phabricator via cfe-commits]

hyklv added a comment.

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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57655



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

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

[Hylke Kleve via Phabricator via cfe-commits]

hyklv updated this revision to Diff 186315.
hyklv marked an inline comment as done.
hyklv added a comment.

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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57655

Files:
  D:/Format/llvm/tools/clang/lib/Format/WhitespaceManager.cpp
  D:/Format/llvm/tools/clang/unittests/Format/FormatTest.cpp


Index: D:/Format/llvm/tools/clang/unittests/Format/FormatTest.cpp
===
--- D:/Format/llvm/tools/clang/unittests/Format/FormatTest.cpp
+++ D:/Format/llvm/tools/clang/unittests/Format/FormatTest.cpp
@@ -8739,6 +8739,9 @@
"\t\tparameter2); \\\n"
"\t}",
Tab);
+  verifyFormat("int a;\t  // x\n"
+   "int ; // x\n",
+   Tab);
 
   Tab.TabWidth = 4;
   Tab.IndentWidth = 8;
Index: D:/Format/llvm/tools/clang/lib/Format/WhitespaceManager.cpp
===
--- D:/Format/llvm/tools/clang/lib/Format/WhitespaceManager.cpp
+++ D:/Format/llvm/tools/clang/lib/Format/WhitespaceManager.cpp
@@ -679,13 +679,17 @@
   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) {
+// Insert only spaces when we want to end up before the next tab.
+if (Spaces < FirstTabWidth || Spaces == 1) {
+  Text.append(Spaces, ' ');
+} else {
+  // Align to next tab.
   Spaces -= FirstTabWidth;
   Text.append("\t");
+
+  Text.append(Spaces / Style.TabWidth, '\t');
+  Text.append(Spaces % Style.TabWidth, ' ');
 }
-Text.append(Spaces / Style.TabWidth, '\t');
-Text.append(Spaces % Style.TabWidth, ' ');
 break;
   }
   case FormatStyle::UT_ForIndentation:


Index: D:/Format/llvm/tools/clang/unittests/Format/FormatTest.cpp
===
--- D:/Format/llvm/tools/clang/unittests/Format/FormatTest.cpp
+++ D:/Format/llvm/tools/clang/unittests/Format/FormatTest.cpp
@@ -8739,6 +8739,9 @@
"\t\tparameter2); \\\n"
"\t}",
Tab);
+  verifyFormat("int a;\t  // x\n"
+   "int ; // x\n",
+   Tab);
 
   Tab.TabWidth = 4;
   Tab.IndentWidth = 8;
Index: D:/Format/llvm/tools/clang/lib/Format/WhitespaceManager.cpp
===
--- D:/Format/llvm/tools/clang/lib/Format/WhitespaceManager.cpp
+++ D:/Format/llvm/tools/clang/lib/Format/WhitespaceManager.cpp
@@ -679,13 +679,17 @@
   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) {
+// Insert only spaces when we want to end up before the next tab.
+if (Spaces < FirstTabWidth || Spaces == 1) {
+  Text.append(Spaces, ' ');
+} else {
+  // Align to next tab.
   Spaces -= FirstTabWidth;
   Text.append("\t");
+
+  Text.append(Spaces / Style.TabWidth, '\t');
+  Text.append(Spaces % Style.TabWidth, ' ');
 }
-Text.append(Spaces / Style.TabWidth, '\t');
-Text.append(Spaces % Style.TabWidth, ' ');
 break;
   }
   case FormatStyle::UT_ForIndentation:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Hylke Kleve via Phabricator via cfe-commits]

hyklv created this revision.
hyklv added reviewers: alexfh, djasper.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Hi,

I'm trying to get clang-format suited for our code layout. I've set it up to 
align declarations and trailing comments. Tab size is 4; UseTab is set to 
Always. Configuration file is pasted below. I see that it misaligns the 
trailing comment for a file with this code:

  struct NavOffMeshGraphEdge
  {
pcRTTIRefObject mObject;  ///< 
The object NPC's will end up using to traverse this edge.
pNavMeshObject  mNavMeshObject{ nullptr };///< The navmesh 
object assigning costs for path finding over this edge.
  };

To align the second trailing comment it needs to insert 4 spaces. This should 
be one tab plus two spaces. It skips the logic of the first partial tab in 
`FirstTabWidth (=2) + Style.TabWidth (=4) <= Spaces (=4)` while it shouldn't. 
Proposed fix is attached.

Our .clang-format is setup with:

  ---
  Language:Cpp
  AccessModifierOffset: -4
  AlignAfterOpenBracket: Align
  AlignConsecutiveAssignments: true
  AlignConsecutiveDeclarations: true
  AlignEscapedNewlines: Right
  AlignOperands:   true
  AlignTrailingComments: true
  AllowAllParametersOfDeclarationOnNextLine: false
  AllowShortBlocksOnASingleLine: true
  AllowShortCaseLabelsOnASingleLine: true
  AllowShortFunctionsOnASingleLine: Inline
  AllowShortIfStatementsOnASingleLine: false
  AllowShortLoopsOnASingleLine: false
  AlwaysBreakAfterDefinitionReturnType: None
  AlwaysBreakAfterReturnType: None
  AlwaysBreakBeforeMultilineStrings: false
  AlwaysBreakTemplateDeclarations: true
  BinPackArguments: true
  BinPackParameters: true
  BraceWrapping:   
AfterClass:  true
AfterControlStatement: true
AfterEnum:   true
AfterFunction:   true
AfterNamespace:  true
AfterObjCDeclaration: false
AfterStruct: true
AfterUnion:  true
BeforeCatch: true
BeforeElse:  true
IndentBraces:false
SplitEmptyFunction: true
SplitEmptyRecord: true
SplitEmptyNamespace: true
  BreakBeforeBinaryOperators: None
  BreakBeforeBraces: Custom
  BreakBeforeInheritanceComma: false
  BreakBeforeTernaryOperators: false
  BreakConstructorInitializersBeforeComma: false
  BreakConstructorInitializers: AfterColon
  BreakAfterJavaFieldAnnotations: false
  BreakStringLiterals: false
  ColumnLimit: 5000
  CommentPragmas:  '^ IWYU pragma:'
  CompactNamespaces: false
  ConstructorInitializerAllOnOneLineOrOnePerLine: true
  ConstructorInitializerIndentWidth: 4
  ContinuationIndentWidth: 4
  Cpp11BracedListStyle: false
  DerivePointerAlignment: false
  DisableFormat:   false
  ExperimentalAutoDetectBinPacking: false
  FixNamespaceComments: false
  ForEachMacros:   
- foreach
- Q_FOREACH
- BOOST_FOREACH
  IncludeCategories: 
- Regex:   '^".*\.h"'
  Priority:1
- Regex:   '^'
  Priority:2
- Regex:   '^<.*\.h>'
  Priority:1
- Regex:   '^<.*'
  Priority:2
- Regex:   '.*'
  Priority:3
  IncludeIsMainRegex: '([-_](test|unittest))?$'
  IndentCaseLabels: false
  IndentWidth: 4
  IndentWrappedFunctionNames: true
  IndentPPDirectives: None
  JavaScriptQuotes: Leave
  JavaScriptWrapImports: true
  KeepEmptyLinesAtTheStartOfBlocks: false
  MacroBlockBegin: "^RTTI_START_"
  MacroBlockEnd: "^\
  RTTI_END"
  MaxEmptyLinesToKeep: 3
  NamespaceIndentation: None
  ObjCBlockIndentWidth: 2
  ObjCSpaceAfterProperty: false
  ObjCSpaceBeforeProtocolList: false
  PenaltyBreakAssignment: 2
  PenaltyBreakBeforeFirstCallParameter: 1
  PenaltyBreakComment: 300
  PenaltyBreakFirstLessLess: 120
  PenaltyBreakString: 1000
  PenaltyExcessCharacter: 100
  PenaltyReturnTypeOnItsOwnLine: 200
  PointerAlignment: Left
  ReflowComments:  true
  SortIncludes:true
  SortUsingDeclarations: true
  SpaceAfterCStyleCast: true
  SpaceAfterTemplateKeyword: true
  SpaceBeforeAssignmentOperators: true
  SpaceBeforeParens: ControlStatements
  SpaceInEmptyParentheses: false
  SpacesBeforeTrailingComments: 4
  SpacesInAngles:  false
  SpacesInContainerLiterals: false
  SpacesInCStyleCastParentheses: false
  SpacesInParentheses: false
  SpacesInSquareBrackets: false
  Standard:Auto
  TabWidth:4
  UseTab:  Always
  ...




Repository:
  rC Clang

https://reviews.llvm.org/D57655

Files:
  WhitespaceManager.cpp


Index: WhitespaceManager.cpp
===
--- WhitespaceManager.cpp
+++ WhitespaceManager.cpp
@@ -1,9 +1,10 @@
 //===--- WhitespaceManager.cpp - Format C++ code 
--===//
 //
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//