[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[Björn Schäpers via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG772eb24e0062: [clang-format] Add option to control the 
spaces in a line comment (authored by HazardyKnusperkeks).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/BreakableToken.cpp
  clang/lib/Format/BreakableToken.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/NamespaceEndCommentsFixer.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/FormatTestComments.cpp

Index: clang/unittests/Format/FormatTestComments.cpp
===
--- clang/unittests/Format/FormatTestComments.cpp
+++ clang/unittests/Format/FormatTestComments.cpp
@@ -3144,7 +3144,7 @@
 "  # commen6\n"
 "  # commen7",
 format("k:val#commen1 commen2\n"
-   " # commen3\n"
+   " #commen3\n"
"# commen4\n"
"a:1#commen5 commen6\n"
" #commen7",
@@ -3275,6 +3275,526 @@
JSStyle20));
 }
 
+TEST_F(FormatTestComments, SpaceAtLineCommentBegin) {
+  FormatStyle Style = getLLVMStyle();
+  StringRef NoTextInComment = " //   \n"
+  "\n"
+  "void foo() {// \n"
+  "// \n"
+  "}";
+
+  EXPECT_EQ("//\n"
+"\n"
+"void foo() { //\n"
+"  //\n"
+"}",
+format(NoTextInComment, Style));
+
+  Style.SpacesInLineCommentPrefix.Minimum = 0;
+  EXPECT_EQ("//\n"
+"\n"
+"void foo() { //\n"
+"  //\n"
+"}",
+format(NoTextInComment, Style));
+
+  Style.SpacesInLineCommentPrefix.Minimum = 5;
+  EXPECT_EQ("//\n"
+"\n"
+"void foo() { //\n"
+"  //\n"
+"}",
+format(NoTextInComment, Style));
+
+  Style = getLLVMStyle();
+  StringRef Code =
+  "//Free comment without space\n"
+  "\n"
+  "//   Free comment with 3 spaces\n"
+  "\n"
+  "///Free Doxygen without space\n"
+  "\n"
+  "///   Free Doxygen with 3 spaces\n"
+  "\n"
+  "/// A Doxygen Comment with a nested list:\n"
+  "/// - Foo\n"
+  "/// - Bar\n"
+  "///   - Baz\n"
+  "///   - End\n"
+  "/// of the inner list\n"
+  "///   .\n"
+  "/// .\n"
+  "\n"
+  "namespace Foo {\n"
+  "bool bar(bool b) {\n"
+  "  bool ret1 = true; ///TokenText;
   if (NamespaceTok->is(TT_NamespaceMacro))
 text += "(";
@@ -278,7 +280,8 @@
   EndCommentNextTok->NewlinesBefore == 0 &&
   EndCommentNextTok->isNot(tok::eof);
 const std::string EndCommentText =
-computeEndCommentText(NamespaceName, AddNewline, NamespaceTok);
+computeEndCommentText(NamespaceName, AddNewline, NamespaceTok,
+  Style.SpacesInLineCommentPrefix.Minimum);
 if (!hasEndComment(EndCommentPrevTok)) {
   bool isShort = I - StartLineIndex <= kShortNamespaceMaxLines + 1;
   if (!isShort)
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -661,6 +661,8 @@
Style.SpacesInContainerLiterals);
 IO.mapOptional("SpacesInCStyleCastParentheses",
Style.SpacesInCStyleCastParentheses);
+IO.mapOptional("SpacesInLineCommentPrefix",
+   Style.SpacesInLineCommentPrefix);
 IO.mapOptional("SpacesInParentheses", Style.SpacesInParentheses);
 IO.mapOptional("SpacesInSquareBrackets", Style.SpacesInSquareBrackets);
 IO.mapOptional("SpaceBeforeSquareBrackets",
@@ -710,6 +712,20 @@
   }
 };
 
+template <> struct MappingTraits {
+  static void mapping(IO &IO, FormatStyle::SpacesInLineComment &Space) {
+// Transform the maximum to signed, to parse "-1" correctly
+int signedMaximum = static_cast(Space.Maximum);
+IO.mapOptional("Minimum", Space.Minimum);
+IO.mapOptional("Maximum", signedMaximum);
+Space.Maximum = static_cast(signedMaximum);
+
+if (Space.Maximum != -1u) {
+  Space.Minimum = std::min(Space.Minimum, Space.Maximum);
+}
+  }
+};
+
 // Allows to read vector while keeping default values.
 // IO.getContext() should contain a pointer to the FormatStyle structure, that
 // will be used to get default values for missing keys.
@@ -986,6 +1002,7 @@
   LLVMStyle.SpaceInEmptyParentheses = false;
   LLVMStyle.SpacesInContainerLiterals = true;
   LLVMStyle.SpacesInCStyleCastParentheses = false;
+  LLVMStyle.SpacesInLineCommentPrefix = {/

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

My assumption is that you want to stick with the minimum and maximum is that 
correct?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

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

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

In D92257#2497535 , @MyDeveloperDay 
wrote:

> My assumption is that you want to stick with the minimum and maximum is that 
> correct?

Otherwise I have to make a breaking change, or not achieve at all what I want. 
So either abandon this or we need a minimum and maximum.
Although right now I have changed an existing test because in that case the 
behavior changed (as noted inline) and a test case still to decide how to 
advance.

If that little break is not acceptable I see no further base to pursue this and 
have to decide to drop it totally or only apply it locally.

And thanks for bringing this up again, currently I have very little time to 
work on clang-format and only react on the mails. Even while I have many things 
I want to add/change or cases where it is currently misformatted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

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

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks updated this revision to Diff 317294.
HazardyKnusperkeks added a comment.

- Rebased
- Fixed(?) the last UnitTest, please take a look @krasimir


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/BreakableToken.cpp
  clang/lib/Format/BreakableToken.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/NamespaceEndCommentsFixer.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/FormatTestComments.cpp

Index: clang/unittests/Format/FormatTestComments.cpp
===
--- clang/unittests/Format/FormatTestComments.cpp
+++ clang/unittests/Format/FormatTestComments.cpp
@@ -3144,7 +3144,7 @@
 "  # commen6\n"
 "  # commen7",
 format("k:val#commen1 commen2\n"
-   " # commen3\n"
+   " #commen3\n"
"# commen4\n"
"a:1#commen5 commen6\n"
" #commen7",
@@ -3275,6 +3275,506 @@
JSStyle20));
 }
 
+TEST_F(FormatTestComments, SpaceAtLineCommentBegin) {
+  FormatStyle Style = getLLVMStyle();
+  StringRef NoTextInComment = " //   \n"
+  "\n"
+  "void foo() {// \n"
+  "// \n"
+  "}";
+
+  EXPECT_EQ("//\n"
+"\n"
+"void foo() { //\n"
+"  //\n"
+"}",
+format(NoTextInComment, Style));
+
+  Style.SpacesInLineCommentPrefix.Minimum = 0;
+  EXPECT_EQ("//\n"
+"\n"
+"void foo() { //\n"
+"  //\n"
+"}",
+format(NoTextInComment, Style));
+
+  Style.SpacesInLineCommentPrefix.Minimum = 5;
+  EXPECT_EQ("//\n"
+"\n"
+"void foo() { //\n"
+"  //\n"
+"}",
+format(NoTextInComment, Style));
+
+  Style = getLLVMStyle();
+  StringRef Code =
+  "//Free comment without space\n"
+  "\n"
+  "//   Free comment with 3 spaces\n"
+  "\n"
+  "///Free Doxygen without space\n"
+  "\n"
+  "///   Free Doxygen with 3 spaces\n"
+  "\n"
+  "/// A Doxygen Comment with a nested list:\n"
+  "/// - Foo\n"
+  "/// - Bar\n"
+  "///   - Baz\n"
+  "///   - End\n"
+  "/// of the inner list\n"
+  "///   .\n"
+  "/// .\n"
+  "\n"
+  "namespace Foo {\n"
+  "bool bar(bool b) {\n"
+  "  bool ret1 = true; ///TokenText;
   if (NamespaceTok->is(TT_NamespaceMacro))
 text += "(";
@@ -278,7 +280,8 @@
   EndCommentNextTok->NewlinesBefore == 0 &&
   EndCommentNextTok->isNot(tok::eof);
 const std::string EndCommentText =
-computeEndCommentText(NamespaceName, AddNewline, NamespaceTok);
+computeEndCommentText(NamespaceName, AddNewline, NamespaceTok,
+  Style.SpacesInLineCommentPrefix.Minimum);
 if (!hasEndComment(EndCommentPrevTok)) {
   bool isShort = I - StartLineIndex <= kShortNamespaceMaxLines + 1;
   if (!isShort)
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -632,6 +632,8 @@
Style.SpacesInContainerLiterals);
 IO.mapOptional("SpacesInCStyleCastParentheses",
Style.SpacesInCStyleCastParentheses);
+IO.mapOptional("SpacesInLineCommentPrefix",
+   Style.SpacesInLineCommentPrefix);
 IO.mapOptional("SpacesInParentheses", Style.SpacesInParentheses);
 IO.mapOptional("SpacesInSquareBrackets", Style.SpacesInSquareBrackets);
 IO.mapOptional("SpaceBeforeSquareBrackets",
@@ -681,6 +683,20 @@
   }
 };
 
+template <> struct MappingTraits {
+  static void mapping(IO &IO, FormatStyle::SpacesInLineComment &Space) {
+// Transform the maximum to signed, to parse "-1" correctly
+int signedMaximum = static_cast(Space.Maximum);
+IO.mapOptional("Minimum", Space.Minimum);
+IO.mapOptional("Maximum", signedMaximum);
+Space.Maximum = static_cast(signedMaximum);
+
+if (Space.Maximum != -1u) {
+  Space.Minimum = std::min(Space.Minimum, Space.Maximum);
+}
+  }
+};
+
 // Allows to read vector while keeping default values.
 // IO.getContext() should contain a pointer to the FormatStyle structure, that
 // will be used to get default values for missing keys.
@@ -955,6 +971,7 @@
   LLVMStyle.SpaceInEmptyParentheses = false;
   LLVMStyle.SpacesInContainerLiterals = true;
   LLVMStyle.SpacesInCStyleCastParentheses = false;
+  LLVMStyle.SpacesInLineCommentPrefix = {/*Minimum=*/1, /*Maximum=*/-1u};
   LLVMStyle.SpaceAfterCStyleCast = false;
   LLVMStyle.SpaceAfterLog

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[Björn Schäpers via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG078f30e04d1f: [clang-format] Add option to control the 
spaces in a line comment (authored by HazardyKnusperkeks).

Changed prior to commit:
  https://reviews.llvm.org/D92257?vs=317294&id=320046#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/BreakableToken.cpp
  clang/lib/Format/BreakableToken.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/NamespaceEndCommentsFixer.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/FormatTestComments.cpp

Index: clang/unittests/Format/FormatTestComments.cpp
===
--- clang/unittests/Format/FormatTestComments.cpp
+++ clang/unittests/Format/FormatTestComments.cpp
@@ -3144,7 +3144,7 @@
 "  # commen6\n"
 "  # commen7",
 format("k:val#commen1 commen2\n"
-   " # commen3\n"
+   " #commen3\n"
"# commen4\n"
"a:1#commen5 commen6\n"
" #commen7",
@@ -3275,6 +3275,506 @@
JSStyle20));
 }
 
+TEST_F(FormatTestComments, SpaceAtLineCommentBegin) {
+  FormatStyle Style = getLLVMStyle();
+  StringRef NoTextInComment = " //   \n"
+  "\n"
+  "void foo() {// \n"
+  "// \n"
+  "}";
+
+  EXPECT_EQ("//\n"
+"\n"
+"void foo() { //\n"
+"  //\n"
+"}",
+format(NoTextInComment, Style));
+
+  Style.SpacesInLineCommentPrefix.Minimum = 0;
+  EXPECT_EQ("//\n"
+"\n"
+"void foo() { //\n"
+"  //\n"
+"}",
+format(NoTextInComment, Style));
+
+  Style.SpacesInLineCommentPrefix.Minimum = 5;
+  EXPECT_EQ("//\n"
+"\n"
+"void foo() { //\n"
+"  //\n"
+"}",
+format(NoTextInComment, Style));
+
+  Style = getLLVMStyle();
+  StringRef Code =
+  "//Free comment without space\n"
+  "\n"
+  "//   Free comment with 3 spaces\n"
+  "\n"
+  "///Free Doxygen without space\n"
+  "\n"
+  "///   Free Doxygen with 3 spaces\n"
+  "\n"
+  "/// A Doxygen Comment with a nested list:\n"
+  "/// - Foo\n"
+  "/// - Bar\n"
+  "///   - Baz\n"
+  "///   - End\n"
+  "/// of the inner list\n"
+  "///   .\n"
+  "/// .\n"
+  "\n"
+  "namespace Foo {\n"
+  "bool bar(bool b) {\n"
+  "  bool ret1 = true; ///TokenText;
   if (NamespaceTok->is(TT_NamespaceMacro))
 text += "(";
@@ -278,7 +280,8 @@
   EndCommentNextTok->NewlinesBefore == 0 &&
   EndCommentNextTok->isNot(tok::eof);
 const std::string EndCommentText =
-computeEndCommentText(NamespaceName, AddNewline, NamespaceTok);
+computeEndCommentText(NamespaceName, AddNewline, NamespaceTok,
+  Style.SpacesInLineCommentPrefix.Minimum);
 if (!hasEndComment(EndCommentPrevTok)) {
   bool isShort = I - StartLineIndex <= kShortNamespaceMaxLines + 1;
   if (!isShort)
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -661,6 +661,8 @@
Style.SpacesInContainerLiterals);
 IO.mapOptional("SpacesInCStyleCastParentheses",
Style.SpacesInCStyleCastParentheses);
+IO.mapOptional("SpacesInLineCommentPrefix",
+   Style.SpacesInLineCommentPrefix);
 IO.mapOptional("SpacesInParentheses", Style.SpacesInParentheses);
 IO.mapOptional("SpacesInSquareBrackets", Style.SpacesInSquareBrackets);
 IO.mapOptional("SpaceBeforeSquareBrackets",
@@ -710,6 +712,20 @@
   }
 };
 
+template <> struct MappingTraits {
+  static void mapping(IO &IO, FormatStyle::SpacesInLineComment &Space) {
+// Transform the maximum to signed, to parse "-1" correctly
+int signedMaximum = static_cast(Space.Maximum);
+IO.mapOptional("Minimum", Space.Minimum);
+IO.mapOptional("Maximum", signedMaximum);
+Space.Maximum = static_cast(signedMaximum);
+
+if (Space.Maximum != -1u) {
+  Space.Minimum = std::min(Space.Minimum, Space.Maximum);
+}
+  }
+};
+
 // Allows to read vector while keeping default values.
 // IO.getContext() should contain a pointer to the FormatStyle structure, that
 // will be used to get default values for missing keys.
@@ -986,6 +1002,7 @@
   LLVMStyle.SpaceInEmptyParentheses = false;
   LLVMStyle.SpacesInContainerLiterals = true;
   LLVMStyle.SpacesInCStyleCastParentheses = false;
+  LLVMStyle.Sp

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir added inline comments.



Comment at: clang/unittests/Format/FormatTestComments.cpp:3141-3147
 "# commen3\n"
 "# commen4\n"
 "a: 1  # commen5\n"
 "  # commen6\n"
 "  # commen7",
 format("k:val#commen1 commen2\n"
" # commen3\n"

HazardyKnusperkeks wrote:
> Here the test fails, because `commen1` gets a space added and `commen3` 
> belongs to the same section, thus also gets an additional space.
> I see three options:
> 
>   # The whole keeping indentation in a section is wrong.
>   # Disable the mechanic for text proto.
>   # Adapt the test.
> 
> 
This test change looks OK.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

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

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks updated this revision to Diff 320196.
HazardyKnusperkeks added a comment.

The previous one broke a (format) test in polly. This lead me to change the one 
breaking behavior, no it is not breaking anymore.
A comment starting with `}` will only be indented if it is in a comment section 
which will get an indention. Test case is adapted.

I will wait a few days if there is no negative feedback I will push it again.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/BreakableToken.cpp
  clang/lib/Format/BreakableToken.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/NamespaceEndCommentsFixer.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/FormatTestComments.cpp

Index: clang/unittests/Format/FormatTestComments.cpp
===
--- clang/unittests/Format/FormatTestComments.cpp
+++ clang/unittests/Format/FormatTestComments.cpp
@@ -3144,7 +3144,7 @@
 "  # commen6\n"
 "  # commen7",
 format("k:val#commen1 commen2\n"
-   " # commen3\n"
+   " #commen3\n"
"# commen4\n"
"a:1#commen5 commen6\n"
" #commen7",
@@ -3275,6 +3275,526 @@
JSStyle20));
 }
 
+TEST_F(FormatTestComments, SpaceAtLineCommentBegin) {
+  FormatStyle Style = getLLVMStyle();
+  StringRef NoTextInComment = " //   \n"
+  "\n"
+  "void foo() {// \n"
+  "// \n"
+  "}";
+
+  EXPECT_EQ("//\n"
+"\n"
+"void foo() { //\n"
+"  //\n"
+"}",
+format(NoTextInComment, Style));
+
+  Style.SpacesInLineCommentPrefix.Minimum = 0;
+  EXPECT_EQ("//\n"
+"\n"
+"void foo() { //\n"
+"  //\n"
+"}",
+format(NoTextInComment, Style));
+
+  Style.SpacesInLineCommentPrefix.Minimum = 5;
+  EXPECT_EQ("//\n"
+"\n"
+"void foo() { //\n"
+"  //\n"
+"}",
+format(NoTextInComment, Style));
+
+  Style = getLLVMStyle();
+  StringRef Code =
+  "//Free comment without space\n"
+  "\n"
+  "//   Free comment with 3 spaces\n"
+  "\n"
+  "///Free Doxygen without space\n"
+  "\n"
+  "///   Free Doxygen with 3 spaces\n"
+  "\n"
+  "/// A Doxygen Comment with a nested list:\n"
+  "/// - Foo\n"
+  "/// - Bar\n"
+  "///   - Baz\n"
+  "///   - End\n"
+  "/// of the inner list\n"
+  "///   .\n"
+  "/// .\n"
+  "\n"
+  "namespace Foo {\n"
+  "bool bar(bool b) {\n"
+  "  bool ret1 = true; ///TokenText;
   if (NamespaceTok->is(TT_NamespaceMacro))
 text += "(";
@@ -278,7 +280,8 @@
   EndCommentNextTok->NewlinesBefore == 0 &&
   EndCommentNextTok->isNot(tok::eof);
 const std::string EndCommentText =
-computeEndCommentText(NamespaceName, AddNewline, NamespaceTok);
+computeEndCommentText(NamespaceName, AddNewline, NamespaceTok,
+  Style.SpacesInLineCommentPrefix.Minimum);
 if (!hasEndComment(EndCommentPrevTok)) {
   bool isShort = I - StartLineIndex <= kShortNamespaceMaxLines + 1;
   if (!isShort)
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -661,6 +661,8 @@
Style.SpacesInContainerLiterals);
 IO.mapOptional("SpacesInCStyleCastParentheses",
Style.SpacesInCStyleCastParentheses);
+IO.mapOptional("SpacesInLineCommentPrefix",
+   Style.SpacesInLineCommentPrefix);
 IO.mapOptional("SpacesInParentheses", Style.SpacesInParentheses);
 IO.mapOptional("SpacesInSquareBrackets", Style.SpacesInSquareBrackets);
 IO.mapOptional("SpaceBeforeSquareBrackets",
@@ -710,6 +712,20 @@
   }
 };
 
+template <> struct MappingTraits {
+  static void mapping(IO &IO, FormatStyle::SpacesInLineComment &Space) {
+// Transform the maximum to signed, to parse "-1" correctly
+int signedMaximum = static_cast(Space.Maximum);
+IO.mapOptional("Minimum", Space.Minimum);
+IO.mapOptional("Maximum", signedMaximum);
+Space.Maximum = static_cast(signedMaximum);
+
+if (Space.Maximum != -1u) {
+  Space.Minimum = std::min(Space.Minimum, Space.Maximum);
+}
+  }
+};
+
 // Allows to read vector while keeping default values.
 // IO.getContext() should contain a pointer to the FormatStyle structure, that
 // will be used to get default values for missing keys.
@@ -986,6 +1002,7 @@
   LLVMStyle.Sp

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[Marek Kurdej via Phabricator via cfe-commits]

curdeius accepted this revision.
curdeius added a comment.

LGTM. Could you please give us a link to the failing test in Polly? May be 
GitHub or buildbot.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

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

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

In D92257#2532071 , @curdeius wrote:

> LGTM. Could you please give us a link to the failing test in Polly? May be 
> GitHub or buildbot.

No problem:

http://lab.llvm.org:8011/#builders/10/builds/2294


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

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

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

In D92257#2532077 , @MyDeveloperDay 
wrote:

> I have a script that runs clang-format -n on various directories in clang
> that are clang format clean, polly is one of them because they have clang
> format as a unit test
>
> I use this to ensure I don’t regress behaviour
>
> Maybe we should formalise this with some sort of clang-format-check cmake
> rule
>
> Mydeveloperday

That would be ok for me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

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

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks created this revision.
HazardyKnusperkeks added reviewers: MyDeveloperDay, klimek, krasimir.
HazardyKnusperkeks added a project: clang-format.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
HazardyKnusperkeks requested review of this revision.

I'm not sure about the name.

And I don't know about changing the Prefix from `StringRef` to `std::string`, 
but I suppose nearly all prefixes should fit into the small buffer, so no heap 
allocation.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D92257

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/BreakableToken.cpp
  clang/lib/Format/BreakableToken.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/NamespaceEndCommentsFixer.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/FormatTestComments.cpp

Index: clang/unittests/Format/FormatTestComments.cpp
===
--- clang/unittests/Format/FormatTestComments.cpp
+++ clang/unittests/Format/FormatTestComments.cpp
@@ -3269,6 +3269,183 @@
JSStyle20));
 }
 
+TEST_F(FormatTestComments, SpaceAtLineCommentBegin) {
+  FormatStyle Style = getLLVMStyle();
+  StringRef NoTextInComment = " // \n"
+  "\n"
+  "void foo() {// \n"
+  "// \n"
+  "}";
+
+  EXPECT_EQ("//\n"
+"\n"
+"void foo() { //\n"
+"  //\n"
+"}",
+format(NoTextInComment, Style));
+
+  Style.SpacesInLineComments.Minimum = 0;
+  EXPECT_EQ("//\n"
+"\n"
+"void foo() { //\n"
+"  //\n"
+"}",
+format(NoTextInComment, Style));
+
+  Style = getLLVMStyle();
+  StringRef Code = "//Free comment without space\n"
+   "//   Free comment with 3 spaces\n"
+   "///Free Doxygen without space\n"
+   "///   Free Doxygen with 3 spaces\n"
+   "namespace Foo {\n"
+   "bool bar(bool b) {\n"
+   "  bool ret1 = true; ///TokenText;
   if (NamespaceTok->is(TT_NamespaceMacro))
 text += "(";
@@ -278,7 +280,8 @@
   EndCommentNextTok->NewlinesBefore == 0 &&
   EndCommentNextTok->isNot(tok::eof);
 const std::string EndCommentText =
-computeEndCommentText(NamespaceName, AddNewline, NamespaceTok);
+computeEndCommentText(NamespaceName, AddNewline, NamespaceTok,
+  Style.SpacesInLineComments.Minimum);
 if (!hasEndComment(EndCommentPrevTok)) {
   bool isShort = I - StartLineIndex <= kShortNamespaceMaxLines + 1;
   if (!isShort)
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -624,6 +624,7 @@
Style.SpacesInContainerLiterals);
 IO.mapOptional("SpacesInCStyleCastParentheses",
Style.SpacesInCStyleCastParentheses);
+IO.mapOptional("SpacesInLineComments", Style.SpacesInLineComments);
 IO.mapOptional("SpacesInParentheses", Style.SpacesInParentheses);
 IO.mapOptional("SpacesInSquareBrackets", Style.SpacesInSquareBrackets);
 IO.mapOptional("SpaceBeforeSquareBrackets",
@@ -673,6 +674,20 @@
   }
 };
 
+template <> struct MappingTraits {
+  static void mapping(IO &IO, FormatStyle::SpacesInLineComment &Space) {
+// Transform the maximum to signed, to parse "-1" correctly
+int signedMaximum = static_cast(Space.Maximum);
+IO.mapOptional("Minimum", Space.Minimum);
+IO.mapOptional("Maximum", signedMaximum);
+Space.Maximum = static_cast(signedMaximum);
+
+if (Space.Maximum != -1u) {
+  Space.Minimum = std::min(Space.Minimum, Space.Maximum);
+}
+  }
+};
+
 // Allows to read vector while keeping default values.
 // IO.getContext() should contain a pointer to the FormatStyle structure, that
 // will be used to get default values for missing keys.
@@ -945,6 +960,7 @@
   LLVMStyle.SpaceInEmptyParentheses = false;
   LLVMStyle.SpacesInContainerLiterals = true;
   LLVMStyle.SpacesInCStyleCastParentheses = false;
+  LLVMStyle.SpacesInLineComments = {/*Minimum=*/1, /*Maximum=*/-1u};
   LLVMStyle.SpaceAfterCStyleCast = false;
   LLVMStyle.SpaceAfterLogicalNot = false;
   LLVMStyle.SpaceAfterTemplateKeyword = true;
Index: clang/lib/Format/BreakableToken.h
===
--- clang/lib/Format/BreakableToken.h
+++ clang/lib/Format/BreakableToken.h
@@ -471,8 +471,9 @@
   // formatting. It can be different than the original prefix when the original
   // line starts like this:
   // //content
-  // Then the original prefix is "//", but the prefix is "// ".
-  SmallVector Prefix;
+  // Then the original prefix is "//", but the prefix could 

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:2727
+
+  * ``unsigned Maximum`` The maximum number of spaces at the start of the 
comment.
+

I'm personally not a massive fan of stuffing -1 into an unsigned but I 
understand why its there, if this was an signed and it was actually -1 would 
the algorithm be substantially worse in your view?



Comment at: clang/lib/Format/BreakableToken.cpp:797
+IndentPrefix
+.drop_back(SpacesInPrefix - Style.SpacesInLineComments.Maximum)
+.str();

my assumption is when Maximum is -1 this is a very large -ve number correct? is 
that defined behavior for drop_back()


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

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

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:2727
+
+  * ``unsigned Maximum`` The maximum number of spaces at the start of the 
comment.
+

MyDeveloperDay wrote:
> I'm personally not a massive fan of stuffing -1 into an unsigned but I 
> understand why its there, if this was an signed and it was actually -1 would 
> the algorithm be substantially worse in your view?
I'm no fan if unsigned in any way, but that seems to be the way in clang-format.
Making it signed would require a few more checks when to use it, but I don't 
see any problem in that.
I just would also make the Minimum signed then just to be consistent.

While parsing the style I would add checks to ensure Minimum is never negative 
and Maximum is always greater or equal to -1, should that print any warnings? 
Is there a standard way of doing so? Or should it be just silently corrected?



Comment at: clang/lib/Format/BreakableToken.cpp:797
+IndentPrefix
+.drop_back(SpacesInPrefix - Style.SpacesInLineComments.Maximum)
+.str();

MyDeveloperDay wrote:
> my assumption is when Maximum is -1 this is a very large -ve number correct? 
> is that defined behavior for drop_back()
Since we check beforehand SpacesInPrefix is larger than Maximum there is no 
problem.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

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

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

I think fundamentally from my perspective this seem ok, out of interest can I 
ask what drove you to require it?

My assumption is that some people write comments like

  // Free comment without space\n"

and you want to be able to consistently format it to be (N spaces, as 
clang-format already does 1 space correct?)

  //  Free comment without space\n"

is that correct? is there a common style guide asking for that? what is the 
rationale




Comment at: clang/lib/Format/BreakableToken.cpp:790
+  (Style.Language != FormatStyle::LK_TextProto ||
+   OriginalPrefix[i].substr(0, 2) != "##")) {
+Prefix[i] = IndentPrefix.str();

is this case covered by a unit test at all? sorry can you explain why you are 
looking for "##"?



Comment at: clang/lib/Format/BreakableToken.cpp:797
+IndentPrefix
+.drop_back(SpacesInPrefix - Style.SpacesInLineComments.Maximum)
+.str();

HazardyKnusperkeks wrote:
> MyDeveloperDay wrote:
> > my assumption is when Maximum is -1 this is a very large -ve number 
> > correct? is that defined behavior for drop_back()
> Since we check beforehand SpacesInPrefix is larger than Maximum there is no 
> problem.
yep sorry didn't see that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

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

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks marked 2 inline comments as done.
HazardyKnusperkeks added a comment.

In D92257#2422381 , @MyDeveloperDay 
wrote:

> I think fundamentally from my perspective this seem ok, out of interest can I 
> ask what drove you to require it?
>
> My assumption is that some people write comments like
>
>   // Free comment without space
>
> and you want to be able to consistently format it to be (N spaces, as 
> clang-format already does 1 space correct?)
>
>   //  Free comment without space
>
> is that correct? is there a common style guide asking for that? what is the 
> rationale

I will go for `{0,0}`, so no space between `//` and the text, I don't know 
about a style guide asking for it, other than my own. (Which I can dictate in 
my company.) :)
I have just recently started using clang-format and it does not everything the 
the way I want to, on some aspects I have adapted, but on others I try to "fix" 
it, you can expect some more changes from me in the next time.




Comment at: clang/lib/Format/BreakableToken.cpp:790
+  (Style.Language != FormatStyle::LK_TextProto ||
+   OriginalPrefix[i].substr(0, 2) != "##")) {
+Prefix[i] = IndentPrefix.str();

MyDeveloperDay wrote:
> is this case covered by a unit test at all? sorry can you explain why you are 
> looking for "##"?
It is covered by multiple tests, that's how I was made aware of it. :)
If you look at the code before it only adds a space if the old prefix is "#" 
not "##" which is also found by `getLineCommentIndentPrefix`. As it seems in 
`TextProto` "##" should not be touched. I can of course add a test in my test 
function.

Now I see a change, in the code before "#" was only accepted when the language 
is `TextProto`, now it is always. But I think for that to happen the parser (or 
lexer?) should have assigned something starting with"#" as comment, right? But 
I can change that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

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

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added inline comments.



Comment at: clang/lib/Format/BreakableToken.cpp:790
+  (Style.Language != FormatStyle::LK_TextProto ||
+   OriginalPrefix[i].substr(0, 2) != "##")) {
+Prefix[i] = IndentPrefix.str();

HazardyKnusperkeks wrote:
> MyDeveloperDay wrote:
> > is this case covered by a unit test at all? sorry can you explain why you 
> > are looking for "##"?
> It is covered by multiple tests, that's how I was made aware of it. :)
> If you look at the code before it only adds a space if the old prefix is "#" 
> not "##" which is also found by `getLineCommentIndentPrefix`. As it seems in 
> `TextProto` "##" should not be touched. I can of course add a test in my test 
> function.
> 
> Now I see a change, in the code before "#" was only accepted when the 
> language is `TextProto`, now it is always. But I think for that to happen the 
> parser (or lexer?) should have assigned something starting with"#" as 
> comment, right? But I can change that.
Okay # # is formatted, I try again:
If you look at the code before it only adds a space if the old prefix is "#" 
not "`##`" which is also found by `getLineCommentIndentPrefix`. As it seems in 
`TextProto` "`##`" should not be touched.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

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

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:2710
 
+**SpacesInLineComments** (``SpacesInLineComment``)
+  How many spaces are allowed at the start of a line comment. To disable the

Is this change generated? with clang/doc/tools/dump_style.py or did you hand 
craft it?

ClangFormatStyleOptions.rst is always autogenerated from running dump_style.py, 
any text you want in the rst needs to be present in Format.h


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

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

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:2710
 
+**SpacesInLineComments** (``SpacesInLineComment``)
+  How many spaces are allowed at the start of a line comment. To disable the

MyDeveloperDay wrote:
> Is this change generated? with clang/doc/tools/dump_style.py or did you hand 
> craft it?
> 
> ClangFormatStyleOptions.rst is always autogenerated from running 
> dump_style.py, any text you want in the rst needs to be present in Format.h
Yes it is generated, after you told me what I have to do on D91507 I have (and 
will in the future) always run that script, I've not touched the file directly.

But I also have not really looked at it, because it is generated. If it is a 
bit odd I can retake a look on other options with nested entries.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

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

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay accepted this revision.
MyDeveloperDay added a comment.
This revision is now accepted and ready to land.

This LGTM, I'm not sure if others have any comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

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

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added inline comments.



Comment at: clang/lib/Format/Format.cpp:963
   LLVMStyle.SpacesInCStyleCastParentheses = false;
+  LLVMStyle.SpacesInLineComments = {/*Minimum=*/1, /*Maximum=*/-1u};
   LLVMStyle.SpaceAfterCStyleCast = false;

I don't know precisely the LLVM style but does it allow more than one space (as 
Maximum would suggest)?
Are there any tests covering that?
And what about other styles, no need to set min/max for them?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

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

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added inline comments.



Comment at: clang/lib/Format/Format.cpp:963
   LLVMStyle.SpacesInCStyleCastParentheses = false;
+  LLVMStyle.SpacesInLineComments = {/*Minimum=*/1, /*Maximum=*/-1u};
   LLVMStyle.SpaceAfterCStyleCast = false;

curdeius wrote:
> I don't know precisely the LLVM style but does it allow more than one space 
> (as Maximum would suggest)?
> Are there any tests covering that?
> And what about other styles, no need to set min/max for them?
The part with the LLVM Style from my test case did run exactly so without any 
modification, so yes it allows more than one space.

Since there was no option before (that I'm aware of) all other styles behaved 
exactly like that. I did not check the style guides if they say anything about 
that, I just preserved the old behavior when nothing is configured.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

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

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[Marek Kurdej via Phabricator via cfe-commits]

curdeius accepted this revision.
curdeius added a comment.

LGTM




Comment at: clang/lib/Format/Format.cpp:963
   LLVMStyle.SpacesInCStyleCastParentheses = false;
+  LLVMStyle.SpacesInLineComments = {/*Minimum=*/1, /*Maximum=*/-1u};
   LLVMStyle.SpaceAfterCStyleCast = false;

HazardyKnusperkeks wrote:
> curdeius wrote:
> > I don't know precisely the LLVM style but does it allow more than one space 
> > (as Maximum would suggest)?
> > Are there any tests covering that?
> > And what about other styles, no need to set min/max for them?
> The part with the LLVM Style from my test case did run exactly so without any 
> modification, so yes it allows more than one space.
> 
> Since there was no option before (that I'm aware of) all other styles behaved 
> exactly like that. I did not check the style guides if they say anything 
> about that, I just preserved the old behavior when nothing is configured.
Ok. Great


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

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

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

Can I assume you need someone to land this for you?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

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

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

In D92257#2435701 , @MyDeveloperDay 
wrote:

> Can I assume you need someone to land this for you?

Yes I do. But I have a question, my last change is commited in your name, that 
means git blame would blame it on you, right?

You can set me as author:
`Björn Schäpers `
My Github Account is also called `HazardyKnusperkeks`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

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

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D92257#2435899 , 
@HazardyKnusperkeks wrote:

> In D92257#2435701 , @MyDeveloperDay 
> wrote:
>
>> Can I assume you need someone to land this for you?
>
> Yes I do. But I have a question, my last change is commited in your name, 
> that means git blame would blame it on you, right?
>
> You can set me as author:
> `Björn Schäpers `
> My Github Account is also called `HazardyKnusperkeks`.

The process is that you add (https://llvm.org/docs/Contributing.html)

Patch By: HazardyKnusperkeks

to the commit message if the user doesn't have commit access, if you want your 
name against the blame then I recommend applying for commit access yourself.

let me know if you still want me to land this


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

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

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

In D92257#2435902 , @MyDeveloperDay 
wrote:

> In D92257#2435899 , 
> @HazardyKnusperkeks wrote:
>
>> In D92257#2435701 , @MyDeveloperDay 
>> wrote:
>>
>>> Can I assume you need someone to land this for you?
>>
>> Yes I do. But I have a question, my last change is commited in your name, 
>> that means git blame would blame it on you, right?
>>
>> You can set me as author:
>> `Björn Schäpers `
>> My Github Account is also called `HazardyKnusperkeks`.
>
> The process is that you add (https://llvm.org/docs/Contributing.html)
>
> Patch By: HazardyKnusperkeks
>
> to the commit message if the user doesn't have commit access, if you want 
> your name against the blame then I recommend applying for commit access 
> yourself.
>
> let me know if you still want me to land this

Updated. :)
Yes please land this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

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

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D92257#2435902 , @MyDeveloperDay 
wrote:

> In D92257#2435899 , 
> @HazardyKnusperkeks wrote:
>
>> In D92257#2435701 , @MyDeveloperDay 
>> wrote:
>>
>>> Can I assume you need someone to land this for you?
>>
>> Yes I do. But I have a question, my last change is commited in your name, 
>> that means git blame would blame it on you, right?
>>
>> You can set me as author:
>> `Björn Schäpers `
>> My Github Account is also called `HazardyKnusperkeks`.
>
> The process is that you add (https://llvm.org/docs/Contributing.html)
>
> Patch By: HazardyKnusperkeks
>
> to the commit message if the user doesn't have commit access, if you want 
> your name against the blame then I recommend applying for commit access 
> yourself.

That is incorrect and does not represent the nowadays reality, i suggest that 
you look up the docs.

> let me know if you still want me to land this


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

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

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:2727
+
+  * ``unsigned Maximum`` The maximum number of spaces at the start of the 
comment.
+

HazardyKnusperkeks wrote:
> MyDeveloperDay wrote:
> > I'm personally not a massive fan of stuffing -1 into an unsigned but I 
> > understand why its there, if this was an signed and it was actually -1 
> > would the algorithm be substantially worse in your view?
> I'm no fan if unsigned in any way, but that seems to be the way in 
> clang-format.
> Making it signed would require a few more checks when to use it, but I don't 
> see any problem in that.
> I just would also make the Minimum signed then just to be consistent.
> 
> While parsing the style I would add checks to ensure Minimum is never 
> negative and Maximum is always greater or equal to -1, should that print any 
> warnings? Is there a standard way of doing so? Or should it be just silently 
> corrected?
I find it confusing why we have 2, Minimum and Maximum, instead of a single one.
I'm not convinced that `Maximum` is useful.
Conceptually I'd prefer a single integer option, say `LineCommentContentIndent` 
that would indicate the default indent used for the content of line comments. 
I'd naively expect `LineCommentContentIndent = `:
* 0 would produce `//comment`
* 1 would produce `// comment` (current default)
* 2 would produce `//  comment`, etc.
and this will work with if the input is any of `//comment`, `// comment`, or 
`//  comment`, etc.

An additional consideration is that line comment sections often contain 
additional indentation, e.g. when there is a bullet list, paragraphs, etc. and 
so we can't guarantee that the indent of each line comment will be less than 
Maximum in general. I'd expect this feature to not adjust extra indent in 
comments, e.g.,
```
// Lorem ipsum dolor sit amet,
//  consectetur adipiscing elit,
//  ...
```
after reformatting with `LineCommentContentIndent=0` to produce
```
//Lorem ipsum dolor sit amet,
// consectetur adipiscing elit,
// ...
```
(and vice-versa, after reformatting with `LineCommentContentIndent=1`).
This may well be handled by code, I just wasn't sure by looking at the code and 
test examples.



Comment at: clang/lib/Format/BreakableToken.cpp:790
+  (Style.Language != FormatStyle::LK_TextProto ||
+   OriginalPrefix[i].substr(0, 2) != "##")) {
+Prefix[i] = IndentPrefix.str();

HazardyKnusperkeks wrote:
> HazardyKnusperkeks wrote:
> > MyDeveloperDay wrote:
> > > is this case covered by a unit test at all? sorry can you explain why you 
> > > are looking for "##"?
> > It is covered by multiple tests, that's how I was made aware of it. :)
> > If you look at the code before it only adds a space if the old prefix is 
> > "#" not "##" which is also found by `getLineCommentIndentPrefix`. As it 
> > seems in `TextProto` "##" should not be touched. I can of course add a test 
> > in my test function.
> > 
> > Now I see a change, in the code before "#" was only accepted when the 
> > language is `TextProto`, now it is always. But I think for that to happen 
> > the parser (or lexer?) should have assigned something starting with"#" as 
> > comment, right? But I can change that.
> Okay # # is formatted, I try again:
> If you look at the code before it only adds a space if the old prefix is "#" 
> not "`##`" which is also found by `getLineCommentIndentPrefix`. As it seems 
> in `TextProto` "`##`" should not be touched.
Thanks for the analysis!
I wrote the text proto comment detection. I believe the current clang-format is 
buggy in that it should transform `##comment` into `## comment` for text proto 
(and similarly for all other `KnownTextProtoPrefixes` in 
`getLineCommentIndentCommentPrefix`), so this `substr(0, 2) != "##"` is 
unnecessary and I should go ahead and update and add tests for that.



Comment at: clang/unittests/Format/FormatTestComments.cpp:3405
+"//  Lorem   ipsum\n"
+"//  dolor   sit amet\n" // Why are here the spaces dropped?
+"\n"

This is desired, AFAIK, and due to the normalization behavior while reflowing: 
when a comment line exceeds the comment limit and is broken up into a new line, 
the full range of blanks is replaced with a newline. 
(https://github.com/llvm/llvm-project/blob/ddb002d7c74c038b64dd9d3c3e4a4b58795cf1a6/clang/lib/Format/BreakableToken.cpp#L66).
Note that reflowing copies the extra indent of the line, e.g.,
```
// line limit  V
// heading
// *line is
//   long long long long 
```
get reformatted as
```
// line limit  V
// heading
// *line is
//   long long
//   long long 
```
so if for ranges of blanks longer of size S>1 we copied the (S-1) blanks at the 
beginning of the next line, we would have cascading comment reflows undesired 
with longer and longer indents.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  http

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

I tend to agree with @krasimir I don't see where you really use Maximum to mean 
anything, the nested configuration seems perhaps unnecessarily confusing?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

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

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks marked 6 inline comments as done.
HazardyKnusperkeks added a comment.

In D92257#2435906 , @lebedev.ri wrote:

> In D92257#2435902 , @MyDeveloperDay 
> wrote:
>
>> In D92257#2435899 , 
>> @HazardyKnusperkeks wrote:
>>
>>> In D92257#2435701 , 
>>> @MyDeveloperDay wrote:
>>>
 Can I assume you need someone to land this for you?
>>>
>>> Yes I do. But I have a question, my last change is commited in your name, 
>>> that means git blame would blame it on you, right?
>>>
>>> You can set me as author:
>>> `Björn Schäpers `
>>> My Github Account is also called `HazardyKnusperkeks`.
>>
>> The process is that you add (https://llvm.org/docs/Contributing.html)
>>
>> Patch By: HazardyKnusperkeks
>>
>> to the commit message if the user doesn't have commit access, if you want 
>> your name against the blame then I recommend applying for commit access 
>> yourself.
>
> That is incorrect and does not represent the nowadays reality, i suggest that 
> you look up the docs.
>
>> let me know if you still want me to land this

What/Where are the docs? I read https://llvm.org/docs/Contributing.html before 
hand and just now https://llvm.org/docs/CodeReview.html

---

I will update the patch, but that won't happen before the weekend.




Comment at: clang/docs/ClangFormatStyleOptions.rst:2727
+
+  * ``unsigned Maximum`` The maximum number of spaces at the start of the 
comment.
+

krasimir wrote:
> HazardyKnusperkeks wrote:
> > MyDeveloperDay wrote:
> > > I'm personally not a massive fan of stuffing -1 into an unsigned but I 
> > > understand why its there, if this was an signed and it was actually -1 
> > > would the algorithm be substantially worse in your view?
> > I'm no fan if unsigned in any way, but that seems to be the way in 
> > clang-format.
> > Making it signed would require a few more checks when to use it, but I 
> > don't see any problem in that.
> > I just would also make the Minimum signed then just to be consistent.
> > 
> > While parsing the style I would add checks to ensure Minimum is never 
> > negative and Maximum is always greater or equal to -1, should that print 
> > any warnings? Is there a standard way of doing so? Or should it be just 
> > silently corrected?
> I find it confusing why we have 2, Minimum and Maximum, instead of a single 
> one.
> I'm not convinced that `Maximum` is useful.
> Conceptually I'd prefer a single integer option, say 
> `LineCommentContentIndent` that would indicate the default indent used for 
> the content of line comments. I'd naively expect `LineCommentContentIndent = 
> `:
> * 0 would produce `//comment`
> * 1 would produce `// comment` (current default)
> * 2 would produce `//  comment`, etc.
> and this will work with if the input is any of `//comment`, `// comment`, or 
> `//  comment`, etc.
> 
> An additional consideration is that line comment sections often contain 
> additional indentation, e.g. when there is a bullet list, paragraphs, etc. 
> and so we can't guarantee that the indent of each line comment will be less 
> than Maximum in general. I'd expect this feature to not adjust extra indent 
> in comments, e.g.,
> ```
> // Lorem ipsum dolor sit amet,
> //  consectetur adipiscing elit,
> //  ...
> ```
> after reformatting with `LineCommentContentIndent=0` to produce
> ```
> //Lorem ipsum dolor sit amet,
> // consectetur adipiscing elit,
> // ...
> ```
> (and vice-versa, after reformatting with `LineCommentContentIndent=1`).
> This may well be handled by code, I just wasn't sure by looking at the code 
> and test examples.
I was actually going for only one value, but while writing the tests I came to 
the conclusion that before my change is only enforced a minimum of 1. But that 
very well may be because of what you call line comment sections, I did not 
consider that. That's why I chose a minimum and maximum.

I will modify the patch to one value and will also add tests for the sections. 
But for that I need to remember if I added or removed spaces, right? Is there 
already infrastructure for that? Or is there any documentation of the various 
steps clang-format takes to parse and format code? Until now I tried to 
understand what's going on through single stepping with the debugger (quite 
time consuming).



Comment at: clang/lib/Format/BreakableToken.cpp:790
+  (Style.Language != FormatStyle::LK_TextProto ||
+   OriginalPrefix[i].substr(0, 2) != "##")) {
+Prefix[i] = IndentPrefix.str();

krasimir wrote:
> HazardyKnusperkeks wrote:
> > HazardyKnusperkeks wrote:
> > > MyDeveloperDay wrote:
> > > > is this case covered by a unit test at all? sorry can you explain why 
> > > > you are looking for "##"?
> > > It is covered by multiple tests, that's how I was made aware of it. :)
> > > If you loo

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D92257#2437918 , 
@HazardyKnusperkeks wrote:

> In D92257#2435906 , @lebedev.ri 
> wrote:
>
>> In D92257#2435902 , @MyDeveloperDay 
>> wrote:
>>
>>> In D92257#2435899 , 
>>> @HazardyKnusperkeks wrote:
>>>
 In D92257#2435701 , 
 @MyDeveloperDay wrote:

> Can I assume you need someone to land this for you?

 Yes I do. But I have a question, my last change is commited in your name, 
 that means git blame would blame it on you, right?

 You can set me as author:
 `Björn Schäpers `
 My Github Account is also called `HazardyKnusperkeks`.
>>>
>>> The process is that you add (https://llvm.org/docs/Contributing.html)
>>>
>>> Patch By: HazardyKnusperkeks
>>>
>>> to the commit message if the user doesn't have commit access, if you want 
>>> your name against the blame then I recommend applying for commit access 
>>> yourself.
>>
>> That is incorrect and does not represent the nowadays reality, i suggest 
>> that you look up the docs.
>>
>>> let me know if you still want me to land this
>
> What/Where are the docs? I read https://llvm.org/docs/Contributing.html 
> before hand and just now https://llvm.org/docs/CodeReview.html

My comment was addressed at @MyDeveloperDay
https://llvm.org/docs/DeveloperPolicy.html#commit-messages

  If you’re not the original author, ensure the ‘Author’ property of the commit
  is set to the original author and the ‘Committer’ property is set to yourself.
  You can use a command similar to git commit --amend --author="John Doe 
"
  to correct the author property if it is incorrect.
  See Attribution of Changes for more information including the method
  we used for attribution before the project migrated to git.

IOW @HazardyKnusperkeks's request was correct.

> ---
>
> I will update the patch, but that won't happen before the weekend.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

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

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D92257#2435906 , @lebedev.ri wrote:

> In D92257#2435902 , @MyDeveloperDay 
> wrote:
>
>> In D92257#2435899 , 
>> @HazardyKnusperkeks wrote:
>>
>>> In D92257#2435701 , 
>>> @MyDeveloperDay wrote:
>>>
 Can I assume you need someone to land this for you?
>>>
>>> Yes I do. But I have a question, my last change is commited in your name, 
>>> that means git blame would blame it on you, right?
>>>
>>> You can set me as author:
>>> `Björn Schäpers `
>>> My Github Account is also called `HazardyKnusperkeks`.
>>
>> The process is that you add (https://llvm.org/docs/Contributing.html)
>>
>> Patch By: HazardyKnusperkeks
>>
>> to the commit message if the user doesn't have commit access, if you want 
>> your name against the blame then I recommend applying for commit access 
>> yourself.
>
> That is incorrect and does not represent the nowadays reality, i suggest that 
> you look up the docs.
>
>> let me know if you still want me to land this

Yes I agree I hadn’t seen that the process had changed,

This is one reason why I don’t like landing patches for others, this just 
confirms that in the future I will generally request people apply for commit 
access themselves.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

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

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay requested changes to this revision.
MyDeveloperDay added a comment.
This revision now requires changes to proceed.

Could we consider dropping the maximum?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

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

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks marked an inline comment as done.
HazardyKnusperkeks added a comment.

In D92257#2438926 , @MyDeveloperDay 
wrote:

> In D92257#2435906 , @lebedev.ri 
> wrote:
>
>> In D92257#2435902 , @MyDeveloperDay 
>> wrote:
>>
>>> In D92257#2435899 , 
>>> @HazardyKnusperkeks wrote:
>>>
 In D92257#2435701 , 
 @MyDeveloperDay wrote:

> Can I assume you need someone to land this for you?

 Yes I do. But I have a question, my last change is commited in your name, 
 that means git blame would blame it on you, right?

 You can set me as author:
 `Björn Schäpers `
 My Github Account is also called `HazardyKnusperkeks`.
>>>
>>> The process is that you add (https://llvm.org/docs/Contributing.html)
>>>
>>> Patch By: HazardyKnusperkeks
>>>
>>> to the commit message if the user doesn't have commit access, if you want 
>>> your name against the blame then I recommend applying for commit access 
>>> yourself.
>>
>> That is incorrect and does not represent the nowadays reality, i suggest 
>> that you look up the docs.
>>
>>> let me know if you still want me to land this
>
> Yes I agree I hadn’t seen that the process had changed,
>
> This is one reason why I don’t like landing patches for others, this just 
> confirms that in the future I will generally request people apply for commit 
> access themselves.

And where do I do that? Also I did not think I would not have a chance of 
getting the access so early.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

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

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.



> And where do I do that? Also I did not think I would not have a chance of 
> getting the access so early.

https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

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

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

In D92257#2438928 , @MyDeveloperDay 
wrote:

> Could we consider dropping the maximum?

While rewriting the tests with the unmodified clang-format I just confirmed 
that currently only the minimum of 1 is enforced, there is no maximum. I.e.

  //x
  int x;
  //   y
  int y;

will be formatted as

  // x
  int x;
  //   y
  int y;

So for what I want to do I can:

1. Enforce the Minimum (for LLVM Style 1)
2. Enforce an optional Maximum (but what if the Maximum is set to 0, what I 
want to do, given that the Minimum is 1?)
3. Stay with the current design of a Minimum/Maximum Pair with (practically 
unbounded Maximum)

I would choose #3, in that case I only have to add a test for line comment 
sections (and most likely adapt the implementation).

  /// A List:
  ///  * Foo
  ///  * Bar


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

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

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

This didn't really address the comments, what is the point of the maximum? what 
if the maximum is > the ColumnLimit?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

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

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

In D92257#2452063 , @MyDeveloperDay 
wrote:

> This didn't really address the comments, what is the point of the maximum?

My goal is to remove all spaces between `//` and the content (with the 
exception of comment sections, as I have learned here), **and** do not break 
the current behavior in any way, and currently it seems to work with an 
unlimited maximum, and a minimum of 1.

In D92257#2452063 , @MyDeveloperDay 
wrote:

> what if the maximum is > the ColumnLimit?

Most likely what ever happens now if the space in the comment is larger than 
the ColumnLimit. But one could just remove spaces as needed.
A more interesting question would be `what happens if the minimum is larger 
than the ColumnLimit?`. For that one had to decide which is more important, I 
would go with the ColumnLimit and reduce the minimum, but maybe that could be 
handled with the penalties? Although I have to admit that I don't understand 
where and how they are used.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

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

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks updated this revision to Diff 313774.
HazardyKnusperkeks marked an inline comment as done.
HazardyKnusperkeks edited the summary of this revision.
HazardyKnusperkeks added a comment.

I'm back!
I've reworked the change to correctly(*) work with line comment sections.

*: That is to be discussed, currently there is one change in behavior which is 
not covered through previous tests and one test which is failing. I will 
highlight that in inline comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/BreakableToken.cpp
  clang/lib/Format/BreakableToken.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/NamespaceEndCommentsFixer.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/FormatTestComments.cpp

Index: clang/unittests/Format/FormatTestComments.cpp
===
--- clang/unittests/Format/FormatTestComments.cpp
+++ clang/unittests/Format/FormatTestComments.cpp
@@ -3275,6 +3275,506 @@
JSStyle20));
 }
 
+TEST_F(FormatTestComments, SpaceAtLineCommentBegin) {
+  FormatStyle Style = getLLVMStyle();
+  StringRef NoTextInComment = " //   \n"
+  "\n"
+  "void foo() {// \n"
+  "// \n"
+  "}";
+
+  EXPECT_EQ("//\n"
+"\n"
+"void foo() { //\n"
+"  //\n"
+"}",
+format(NoTextInComment, Style));
+
+  Style.SpacesInLineCommentPrefix.Minimum = 0;
+  EXPECT_EQ("//\n"
+"\n"
+"void foo() { //\n"
+"  //\n"
+"}",
+format(NoTextInComment, Style));
+
+  Style.SpacesInLineCommentPrefix.Minimum = 5;
+  EXPECT_EQ("//\n"
+"\n"
+"void foo() { //\n"
+"  //\n"
+"}",
+format(NoTextInComment, Style));
+
+  Style = getLLVMStyle();
+  StringRef Code =
+  "//Free comment without space\n"
+  "\n"
+  "//   Free comment with 3 spaces\n"
+  "\n"
+  "///Free Doxygen without space\n"
+  "\n"
+  "///   Free Doxygen with 3 spaces\n"
+  "\n"
+  "/// A Doxygen Comment with a nested list:\n"
+  "/// - Foo\n"
+  "/// - Bar\n"
+  "///   - Baz\n"
+  "///   - End\n"
+  "/// of the inner list\n"
+  "///   .\n"
+  "/// .\n"
+  "\n"
+  "namespace Foo {\n"
+  "bool bar(bool b) {\n"
+  "  bool ret1 = true; ///TokenText;
   if (NamespaceTok->is(TT_NamespaceMacro))
 text += "(";
@@ -278,7 +280,8 @@
   EndCommentNextTok->NewlinesBefore == 0 &&
   EndCommentNextTok->isNot(tok::eof);
 const std::string EndCommentText =
-computeEndCommentText(NamespaceName, AddNewline, NamespaceTok);
+computeEndCommentText(NamespaceName, AddNewline, NamespaceTok,
+  Style.SpacesInLineCommentPrefix.Minimum);
 if (!hasEndComment(EndCommentPrevTok)) {
   bool isShort = I - StartLineIndex <= kShortNamespaceMaxLines + 1;
   if (!isShort)
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -633,6 +633,8 @@
Style.SpacesInContainerLiterals);
 IO.mapOptional("SpacesInCStyleCastParentheses",
Style.SpacesInCStyleCastParentheses);
+IO.mapOptional("SpacesInLineCommentPrefix",
+   Style.SpacesInLineCommentPrefix);
 IO.mapOptional("SpacesInParentheses", Style.SpacesInParentheses);
 IO.mapOptional("SpacesInSquareBrackets", Style.SpacesInSquareBrackets);
 IO.mapOptional("SpaceBeforeSquareBrackets",
@@ -682,6 +684,20 @@
   }
 };
 
+template <> struct MappingTraits {
+  static void mapping(IO &IO, FormatStyle::SpacesInLineComment &Space) {
+// Transform the maximum to signed, to parse "-1" correctly
+int signedMaximum = static_cast(Space.Maximum);
+IO.mapOptional("Minimum", Space.Minimum);
+IO.mapOptional("Maximum", signedMaximum);
+Space.Maximum = static_cast(signedMaximum);
+
+if (Space.Maximum != -1u) {
+  Space.Minimum = std::min(Space.Minimum, Space.Maximum);
+}
+  }
+};
+
 // Allows to read vector while keeping default values.
 // IO.getContext() should contain a pointer to the FormatStyle structure, that
 // will be used to get default values for missing keys.
@@ -957,6 +973,7 @@
   LLVMStyle.SpaceInEmptyParentheses = false;
   LLVMStyle.SpacesInContainerLiterals = true;
   LLVMStyle.SpacesInCStyleCastParentheses = false;
+  LLVMStyle.SpacesInLineCommentPrefix = {/*Minimum=*/1, /*Maximum=*/-1u};
   LLVMStyle.SpaceAfterCStyleCast = false;
   LLVMStyle.SpaceAfterLo

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks marked an inline comment as done.
HazardyKnusperkeks added a comment.

In D92257#2452063 , @MyDeveloperDay 
wrote:

> This didn't really address the comments, what is the point of the maximum? 
> what if the maximum is > the ColumnLimit?

Current behavior if there are more spaces than ColumnLimit: Do not format the 
comment at all. Now if the minimum is larger than the ColumnLimit we will obey 
the limit and then normal behavior kicks in, this is also covered in the tests.




Comment at: clang/docs/ClangFormatStyleOptions.rst:2727
+
+  * ``unsigned Maximum`` The maximum number of spaces at the start of the 
comment.
+

HazardyKnusperkeks wrote:
> krasimir wrote:
> > HazardyKnusperkeks wrote:
> > > MyDeveloperDay wrote:
> > > > I'm personally not a massive fan of stuffing -1 into an unsigned but I 
> > > > understand why its there, if this was an signed and it was actually -1 
> > > > would the algorithm be substantially worse in your view?
> > > I'm no fan if unsigned in any way, but that seems to be the way in 
> > > clang-format.
> > > Making it signed would require a few more checks when to use it, but I 
> > > don't see any problem in that.
> > > I just would also make the Minimum signed then just to be consistent.
> > > 
> > > While parsing the style I would add checks to ensure Minimum is never 
> > > negative and Maximum is always greater or equal to -1, should that print 
> > > any warnings? Is there a standard way of doing so? Or should it be just 
> > > silently corrected?
> > I find it confusing why we have 2, Minimum and Maximum, instead of a single 
> > one.
> > I'm not convinced that `Maximum` is useful.
> > Conceptually I'd prefer a single integer option, say 
> > `LineCommentContentIndent` that would indicate the default indent used for 
> > the content of line comments. I'd naively expect `LineCommentContentIndent 
> > = `:
> > * 0 would produce `//comment`
> > * 1 would produce `// comment` (current default)
> > * 2 would produce `//  comment`, etc.
> > and this will work with if the input is any of `//comment`, `// comment`, 
> > or `//  comment`, etc.
> > 
> > An additional consideration is that line comment sections often contain 
> > additional indentation, e.g. when there is a bullet list, paragraphs, etc. 
> > and so we can't guarantee that the indent of each line comment will be less 
> > than Maximum in general. I'd expect this feature to not adjust extra indent 
> > in comments, e.g.,
> > ```
> > // Lorem ipsum dolor sit amet,
> > //  consectetur adipiscing elit,
> > //  ...
> > ```
> > after reformatting with `LineCommentContentIndent=0` to produce
> > ```
> > //Lorem ipsum dolor sit amet,
> > // consectetur adipiscing elit,
> > // ...
> > ```
> > (and vice-versa, after reformatting with `LineCommentContentIndent=1`).
> > This may well be handled by code, I just wasn't sure by looking at the code 
> > and test examples.
> I was actually going for only one value, but while writing the tests I came 
> to the conclusion that before my change is only enforced a minimum of 1. But 
> that very well may be because of what you call line comment sections, I did 
> not consider that. That's why I chose a minimum and maximum.
> 
> I will modify the patch to one value and will also add tests for the 
> sections. But for that I need to remember if I added or removed spaces, 
> right? Is there already infrastructure for that? Or is there any 
> documentation of the various steps clang-format takes to parse and format 
> code? Until now I tried to understand what's going on through single stepping 
> with the debugger (quite time consuming).
> I find it confusing why we have 2, Minimum and Maximum, instead of a single 
> one.
> I'm not convinced that `Maximum` is useful.
> Conceptually I'd prefer a single integer option, say 
> `LineCommentContentIndent` that would indicate the default indent used for 
> the content of line comments. I'd naively expect `LineCommentContentIndent = 
> `:
> * 0 would produce `//comment`
> * 1 would produce `// comment` (current default)
> * 2 would produce `//  comment`, etc.
> and this will work with if the input is any of `//comment`, `// comment`, or 
> `//  comment`, etc.
> 
> An additional consideration is that line comment sections often contain 
> additional indentation, e.g. when there is a bullet list, paragraphs, etc. 
> and so we can't guarantee that the indent of each line comment will be less 
> than Maximum in general. I'd expect this feature to not adjust extra indent 
> in comments, e.g.,
> ```
> // Lorem ipsum dolor sit amet,
> //  consectetur adipiscing elit,
> //  ...
> ```
> after reformatting with `LineCommentContentIndent=0` to produce
> ```
> //Lorem ipsum dolor sit amet,
> // consectetur adipiscing elit,
> // ...
> ```
> (and vice-versa, after reformatting with `LineCommentContentIndent=1`).
> This may well be handled by code, I just wasn't sure by

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[ksyx via Phabricator via cfe-commits]

ksyx added a comment.

In D92257#3004563 , 
@HazardyKnusperkeks wrote:

> In D92257#3003281 , @byronhe wrote:
>
>> Hi guys，i found `SpacesInLineCommentPrefix` does not support other encoding 
>> such as utf8 ，
>> I am curious why there is a `isAlphanumeric` limit in 
>> `BreakableLineCommentSection::BreakableLineCommentSection()` ?
>> I want to make some contribution to make it support utf8, what should i do ?
>
> The `isAlphanumeric` is there to not break doxygen like comments for example.
>
> I'm very interested in how you want to tackle that problem. :)

Sorry for digging out this month long thing but I encountered this in one 
project full of non-alphanumeric comment. If I understood the problem 
correctly, is it more like avoiding symbols like @ / * rather than avoiding 
non-ASCII characters like CJK's? In this case, would just not adding space when 
it is started with symbols make this option usable for wider amount of comment 
content? :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

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

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[byronhe via Phabricator via cfe-commits]

byronhe added a comment.

Hi guys，i found `SpacesInLineCommentPrefix` does not support other encoding 
such as utf8 ，
I am curious why there is a `isAlphanumeric` limit in 
`BreakableLineCommentSection::BreakableLineCommentSection()` ?
I want to make some contribution to make it support utf8, what should i do ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

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

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D92257#3003281 , @byronhe wrote:

> Hi guys，i found `SpacesInLineCommentPrefix` does not support other encoding 
> such as utf8 ，
> I am curious why there is a `isAlphanumeric` limit in 
> `BreakableLineCommentSection::BreakableLineCommentSection()` ?
> I want to make some contribution to make it support utf8, what should i do ?

see https://llvm.org/docs/Contributing.html but in essence:

1. open a bug at https://bugs.llvm.org/
2. clone the llvm repo from gitub
3. build the repo
4. add unit tests in clang/unittests/Format that show the problem
5. add the code that fixes the issue
6. upload a diff of the patch to reviews.llvm.org
7. add clang-format project tag and at least me as a reviewer and I can help 
fill in the rest

This sounds like a great new contributor idea.. go for it! I'll support this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

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

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

I missed the most important step!

8. Add LLVM contributor to your CV.

No seriously I mean it. I interview people all the time, if I saw that on a CV, 
it would immediately start a conversation about what/who/why you did it. 
(allowing me to look up your contribution)

As an interviewer, Contributing to open source is a great thing!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

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

[PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

In D92257#3003281 , @byronhe wrote:

> Hi guys，i found `SpacesInLineCommentPrefix` does not support other encoding 
> such as utf8 ，
> I am curious why there is a `isAlphanumeric` limit in 
> `BreakableLineCommentSection::BreakableLineCommentSection()` ?
> I want to make some contribution to make it support utf8, what should i do ?

The `isAlphanumeric` is there to not break doxygen like comments for example.

I'm very interested in how you want to tackle that problem. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92257

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

Re: [PATCH] D92257: [clang-format] Add option to control the space at the front of a line comment

[MyDeveloper Day via cfe-commits]

I have a script that runs clang-format -n on various directories in clang
that are clang format clean, polly is one of them because they have clang
format as a unit test

I use this to ensure I don’t regress behaviour

Maybe we should formalise this with some sort of clang-format-check cmake
rule

Mydeveloperday

On Sat, 30 Jan 2021 at 10:04, Björn Schäpers via Phabricator <
revi...@reviews.llvm.org> wrote:

> HazardyKnusperkeks added a comment.
>
> In D92257#2532071 , @curdeius
> wrote:
>
> > LGTM. Could you please give us a link to the failing test in Polly? May
> be GitHub or buildbot.
>
> No problem:
>
> http://lab.llvm.org:8011/#builders/10/builds/2294
>
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D92257/new/
>
> https://reviews.llvm.org/D92257
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits