[PATCH] D53072: [clang-format] Create a new tool for IDEs based on clang-format

[Ivan Donchevskii via Phabricator via cfe-commits]

yvvan updated this revision to Diff 199151.
yvvan added a comment.

Some misleading reformatting fixed.


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

https://reviews.llvm.org/D53072

Files:
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/Format.cpp
  clang/lib/Format/FormatInternal.h
  clang/lib/Format/TokenAnalyzer.cpp
  clang/lib/Format/TokenAnalyzer.h
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/lib/Format/UnwrappedLineFormatter.h
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/lib/Format/UnwrappedLineParser.h
  clang/tools/clang-format/ClangFormat.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -35,15 +35,16 @@
 SC_DoNotCheck
   };
 
-  std::string format(llvm::StringRef Code,
- const FormatStyle  = getLLVMStyle(),
- StatusCheck CheckComplete = SC_ExpectComplete) {
+  std::string
+  format(llvm::StringRef Code, const FormatStyle  = getLLVMStyle(),
+ StatusCheck CheckComplete = SC_ExpectComplete,
+ ExtraFormattingOptions ExtraOptions = ExtraFormattingOptions::None) {
 LLVM_DEBUG(llvm::errs() << "---\n");
 LLVM_DEBUG(llvm::errs() << Code << "\n\n");
 std::vector Ranges(1, tooling::Range(0, Code.size()));
 FormattingAttemptStatus Status;
 tooling::Replacements Replaces =
-reformat(Style, Code, Ranges, "", );
+reformat(Style, ExtraOptions, Code, Ranges, "", );
 if (CheckComplete != SC_DoNotCheck) {
   bool ExpectedCompleteFormat = CheckComplete == SC_ExpectComplete;
   EXPECT_EQ(ExpectedCompleteFormat, Status.FormatComplete)
@@ -395,6 +396,25 @@
Style));
 }
 
+TEST_F(FormatTest, KeepsLineBreaks) {
+  FormatStyle Style = getLLVMStyle();
+  EXPECT_EQ("if (a\n"
+"&& b) {\n"
+"}",
+format("if (a\n"
+   "&& b) {\n"
+   "}",
+   Style, SC_ExpectComplete,
+   ExtraFormattingOptions::KeepLineBreaksForNonEmptyLines));
+
+  EXPECT_EQ("[]() {\n"
+"  foo(); }",
+format("[]() {\n"
+   "foo(); }",
+   Style, SC_ExpectComplete,
+   ExtraFormattingOptions::KeepLineBreaksForNonEmptyLines));
+}
+
 TEST_F(FormatTest, RecognizesBinaryOperatorKeywords) {
   verifyFormat("x = (a) and (b);");
   verifyFormat("x = (a) or (b);");
Index: clang/tools/clang-format/ClangFormat.cpp
===
--- clang/tools/clang-format/ClangFormat.cpp
+++ clang/tools/clang-format/ClangFormat.cpp
@@ -51,13 +51,14 @@
  "Can only be used with one input file."),
 cl::cat(ClangFormatCategory));
 static cl::list
-LineRanges("lines", cl::desc(": - format a range of\n"
- "lines (both 1-based).\n"
- "Multiple ranges can be formatted by specifying\n"
- "several -lines arguments.\n"
- "Can't be used with -offset and -length.\n"
- "Can only be used with one input file."),
-   cl::cat(ClangFormatCategory));
+LineRanges("lines",
+   cl::desc(": - format a range of\n"
+"lines (both 1-based).\n"
+"Multiple ranges can be formatted by specifying\n"
+"several -lines arguments.\n"
+"Can't be used with -offset and -length.\n"
+"Can only be used with one input file."),
+   cl::cat(ClangFormatCategory));
 static cl::opt
 Style("style", cl::desc(clang::format::StyleOptionHelpDescription),
   cl::init(clang::format::DefaultFormatStyle),
@@ -72,12 +73,12 @@
   cl::init(clang::format::DefaultFallbackStyle),
   cl::cat(ClangFormatCategory));
 
-static cl::opt
-AssumeFileName("assume-filename",
-   cl::desc("When reading from stdin, clang-format assumes this\n"
-"filename to look for a style config file (with\n"
-"-style=file) and to determine the language."),
-   cl::init(""), cl::cat(ClangFormatCategory));
+static cl::opt AssumeFileName(
+"assume-filename",
+cl::desc("When reading from stdin, clang-format assumes this\n"
+ "filename to look for a style config file (with\n"
+ "-style=file) and to determine the language."),
+cl::init(""), cl::cat(ClangFormatCategory));
 
 static cl::opt Inplace("i",
  cl::desc("Inplace edit s, if specified."),
@@ -107,6 +108,11 @@
 Verbose("verbose", cl::desc("If set, shows the list of processed files"),
  

[PATCH] D53072: [clang-format] Create a new tool for IDEs based on clang-format

[Ivan Donchevskii via Phabricator via cfe-commits]

yvvan updated this revision to Diff 199150.
yvvan added a comment.

Sorry for unrelated formatting changes - that's clang-format's work :)

I've removed the extra executable.

I don't know how to force that behavior only for the given line (for that I 
need someone who can help) but in our usecase we should not care if we apply 
the rule to the whole file or only to the current line since we are only 
interested in one single Replacement which we can anyways filter afterwards.


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

https://reviews.llvm.org/D53072

Files:
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/Format.cpp
  clang/lib/Format/FormatInternal.h
  clang/lib/Format/TokenAnalyzer.cpp
  clang/lib/Format/TokenAnalyzer.h
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/lib/Format/UnwrappedLineFormatter.h
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/lib/Format/UnwrappedLineParser.h
  clang/tools/clang-format/ClangFormat.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -29,21 +29,18 @@
 
 class FormatTest : public ::testing::Test {
 protected:
-  enum StatusCheck {
-SC_ExpectComplete,
-SC_ExpectIncomplete,
-SC_DoNotCheck
-  };
+  enum StatusCheck { SC_ExpectComplete, SC_ExpectIncomplete, SC_DoNotCheck };
 
-  std::string format(llvm::StringRef Code,
- const FormatStyle  = getLLVMStyle(),
- StatusCheck CheckComplete = SC_ExpectComplete) {
+  std::string
+  format(llvm::StringRef Code, const FormatStyle  = getLLVMStyle(),
+ StatusCheck CheckComplete = SC_ExpectComplete,
+ ExtraFormattingOptions ExtraOptions = ExtraFormattingOptions::None) {
 LLVM_DEBUG(llvm::errs() << "---\n");
 LLVM_DEBUG(llvm::errs() << Code << "\n\n");
 std::vector Ranges(1, tooling::Range(0, Code.size()));
 FormattingAttemptStatus Status;
 tooling::Replacements Replaces =
-reformat(Style, Code, Ranges, "", );
+reformat(Style, ExtraOptions, Code, Ranges, "", );
 if (CheckComplete != SC_DoNotCheck) {
   bool ExpectedCompleteFormat = CheckComplete == SC_ExpectComplete;
   EXPECT_EQ(ExpectedCompleteFormat, Status.FormatComplete)
@@ -332,13 +329,15 @@
 format("namespace {\n"
"int i;\n"
"\n"
-   "}", LLVMWithNoNamespaceFix));
+   "}",
+   LLVMWithNoNamespaceFix));
   EXPECT_EQ("namespace {\n"
 "int i;\n"
 "}",
 format("namespace {\n"
"int i;\n"
-   "}", LLVMWithNoNamespaceFix));
+   "}",
+   LLVMWithNoNamespaceFix));
   EXPECT_EQ("namespace {\n"
 "int i;\n"
 "\n"
@@ -346,13 +345,15 @@
 format("namespace {\n"
"int i;\n"
"\n"
-   "};", LLVMWithNoNamespaceFix));
+   "};",
+   LLVMWithNoNamespaceFix));
   EXPECT_EQ("namespace {\n"
 "int i;\n"
 "};",
 format("namespace {\n"
"int i;\n"
-   "};", LLVMWithNoNamespaceFix));
+   "};",
+   LLVMWithNoNamespaceFix));
   EXPECT_EQ("namespace {\n"
 "int i;\n"
 "\n"
@@ -395,6 +396,25 @@
Style));
 }
 
+TEST_F(FormatTest, KeepsLineBreaks) {
+  FormatStyle Style = getLLVMStyle();
+  EXPECT_EQ("if (a\n"
+"&& b) {\n"
+"}",
+format("if (a\n"
+   "&& b) {\n"
+   "}",
+   Style, SC_ExpectComplete,
+   ExtraFormattingOptions::KeepLineBreaksForNonEmptyLines));
+
+  EXPECT_EQ("[]() {\n"
+"  foo(); }",
+format("[]() {\n"
+   "foo(); }",
+   Style, SC_ExpectComplete,
+   ExtraFormattingOptions::KeepLineBreaksForNonEmptyLines));
+}
+
 TEST_F(FormatTest, RecognizesBinaryOperatorKeywords) {
   verifyFormat("x = (a) and (b);");
   verifyFormat("x = (a) or (b);");
@@ -854,7 +874,8 @@
   verifyFormat("for (Foo *x = 0; x != in; x++) {\n}");
   verifyFormat("Foo *x;\nfor (x = 0; x != in; x++) {\n}");
   verifyFormat("Foo *x;\nfor (x in y) {\n}");
-  verifyFormat("for (const Foo  = in.value(); !baz.at_end(); ++baz) {\n}");
+  verifyFormat(
+  "for (const Foo  = in.value(); !baz.at_end(); ++baz) {\n}");
 
   FormatStyle NoBinPacking = getLLVMStyle();
   NoBinPacking.BinPackParameters = false;
@@ -1246,20 +1267,26 @@
"#endif\n"
"}",
Style));
-  EXPECT_EQ("switch (a) {\n" "case 0:\n"
-"  return; // long long long long long 

[PATCH] D53072: [clang-format] Create a new tool for IDEs based on clang-format

[Ivan Donchevskii via Phabricator via cfe-commits]

yvvan added a comment.

@sammccall 
I can't avoid adding extra formatting options because my first attempt to 
introduce an ordinary clang-format option faced resistance because of not 
fitting the clang-format purpose to format files.


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

https://reviews.llvm.org/D53072



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

[PATCH] D53072: [clang-format] Create a new tool for IDEs based on clang-format

[Ivan Donchevskii via Phabricator via cfe-commits]

yvvan added a comment.

In D53072#1496317 , @sammccall wrote:

> My feedback would be:
>
> - I definitely think more control over preserving line breaks would be 
> useful. Actually preserving *blank* lines is an important property. If you 
> see D60605 , there are a lot of cases where 
> clang-format wants to delete the line you just added. Maybe we can make 
> `MaxEmptyLinesToKeep: 999` work in more cases.
> - I'm not convinced you need/want to preserve *all* line breaks, isn't it 
> just one? (where the user pressed enter)
> - It's unclear why "extraformattingoptions" is needed, rather than just the 
> usual formatting options, or some other parameter. It would be nice to avoid 
> a new library entry point (overload) if possible.
> - as far as command-line interface goes, this definitely feels more like a 
> flag than a new tool


Thanks for the feedback!
'to preserve *all* line breaks, isn't it just one' - it's just much easier code 
wise, you don't need to take the current position into account. I can look how 
I can make it for only one line.
'unclear why "extraformattingoptions" is needed' - the idea behind is that we 
can add more ide-specific options later without introducing new reformat 
parameters.
I will definitely remove a separate tool, it seems nobody sees the need of such 
thing.


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

https://reviews.llvm.org/D53072



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

[PATCH] D53072: [clang-format] Create a new tool for IDEs based on clang-format

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

My feedback would be:

- I definitely think more control over preserving line breaks would be useful. 
Actually preserving *blank* lines is an important property. If you see D60605 
, there are a lot of cases where clang-format 
wants to delete the line you just added. Maybe we can make 
`MaxEmptyLinesToKeep: 999` work in more cases.
- I'm not convinced you need/want to preserve *all* line breaks, isn't it just 
one? (where the user pressed enter)
- It's unclear why "extraformattingoptions" is needed, rather than just the 
usual formatting options, or some other parameter. It would be nice to avoid a 
new library entry point (overload) if possible.
- as far as command-line interface goes, this definitely feels more like a flag 
than a new tool


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

https://reviews.llvm.org/D53072



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

[PATCH] D53072: [clang-format] Create a new tool for IDEs based on clang-format

[Ivan Donchevskii via Phabricator via cfe-commits]

yvvan added a comment.

@ilya-biryukov
I don't really care how it's used from the tool side. I'm also fine to have a 
new option in clang-format itself. That's why this review is here - to ask for 
opinions. It's easy to remove that "ide" part from this patch and just add an 
option for clang-format instead.
Do you have any other remarks/concerns apart from that?


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

https://reviews.llvm.org/D53072



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

[PATCH] D53072: [clang-format] Create a new tool for IDEs based on clang-format

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In D53072#1478575 , @yvvan wrote:

> @sammccall
>
> Having a separate tool is nice because it allows the client to make it 
> plugable. clang-format sometimes changes options quite significantly and it 
> can be nice if you have a choice which version to pick, otherwise it might be 
> unable to read the configuration you have.


I second Sam's concerns about introducing a new tool.
Could you list the use-cases that `clang-format-ide` would cover that 
`clang-format` doesn't?
How is introducing a new tool different from adding a new formatting option to 
`clang-format`?


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

https://reviews.llvm.org/D53072



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

[PATCH] D53072: [clang-format] Create a new tool for IDEs based on clang-format

[Ivan Donchevskii via Phabricator via cfe-commits]

yvvan added a comment.

@sammccall

Having a separate tool is nice because it allows the client to make it 
plugable. clang-format sometimes changes options quite significantly and it can 
be nice if you have a choice which version to pick, otherwise it might be 
unable to read the configuration you have.


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

https://reviews.llvm.org/D53072



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

[PATCH] D53072: [clang-format] Create a new tool for IDEs based on clang-format

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

You may be interested in D60605 , which is a 
related idea (adding incremental format-and-indent-on-type to clangd).

These are opposite extremes in some sense: this patch integrates deeply into 
clang-format and that patch entirely layers on top of it, by transforming the 
source code (in hacky ways, in some cases).

I think the cleanest approach is probably some combination: in particular a 
"preserve line break at point" primitive does seem useful, and even if source 
code transforms are used, putting a nice API on this in clang-format would be 
nice. (I'm not sure a separate command-line-tool is justified, though).

If you haven't run into these yet, here are some fun issues I ran into:

- clang-format bails out when brackets aren't sufficiently matched, incremental 
formatting needs to work in such cases
- incremental formatting sometimes wants to make edits both immediately before 
and after the cursor. `tooling::Replacements` can't represent this, they will 
be merged and destroy precise cursor position information.
- user expectations when breaking between `()` are fairly clear. `{}` is the 
same when it acts as a list, but not when it acts as a block!


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

https://reviews.llvm.org/D53072



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

[PATCH] D53072: [clang-format] Create a new tool for IDEs based on clang-format

[Ivan Donchevskii via Phabricator via cfe-commits]

yvvan updated this revision to Diff 194844.
yvvan retitled this revision from "[clang-format] Introduce the flag which 
allows not to shrink lines" to "[clang-format] Create a new tool for IDEs based 
on clang-format".
yvvan edited the summary of this revision.
yvvan added a reviewer: arphaman.
yvvan added a comment.
Herald added subscribers: dexonsmith, mgorny.

New approach - title and summary are updated.


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

https://reviews.llvm.org/D53072

Files:
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/Format.cpp
  lib/Format/FormatInternal.h
  lib/Format/TokenAnalyzer.cpp
  lib/Format/TokenAnalyzer.h
  lib/Format/UnwrappedLineFormatter.cpp
  lib/Format/UnwrappedLineFormatter.h
  lib/Format/UnwrappedLineParser.cpp
  lib/Format/UnwrappedLineParser.h
  tools/CMakeLists.txt
  tools/clang-format-ide/CMakeLists.txt
  tools/clang-format-ide/ClangFormatIDE.cpp
  tools/clang-format/ClangFormat.cpp
  tools/clang-format/ClangFormat.h
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -35,15 +35,16 @@
 SC_DoNotCheck
   };
 
-  std::string format(llvm::StringRef Code,
- const FormatStyle  = getLLVMStyle(),
- StatusCheck CheckComplete = SC_ExpectComplete) {
+  std::string format(
+  llvm::StringRef Code, const FormatStyle  = getLLVMStyle(),
+  StatusCheck CheckComplete = SC_ExpectComplete,
+  ExtraFormattingOptions FormattingOptions = ExtraFormattingOptions::None) {
 LLVM_DEBUG(llvm::errs() << "---\n");
 LLVM_DEBUG(llvm::errs() << Code << "\n\n");
 std::vector Ranges(1, tooling::Range(0, Code.size()));
 FormattingAttemptStatus Status;
 tooling::Replacements Replaces =
-reformat(Style, Code, Ranges, "", );
+reformat(Style, FormattingOptions, Code, Ranges, "", );
 if (CheckComplete != SC_DoNotCheck) {
   bool ExpectedCompleteFormat = CheckComplete == SC_ExpectComplete;
   EXPECT_EQ(ExpectedCompleteFormat, Status.FormatComplete)
@@ -395,6 +396,25 @@
Style));
 }
 
+TEST_F(FormatTest, KeepsLineBreaks) {
+  FormatStyle Style = getLLVMStyle();
+  EXPECT_EQ("if (a\n"
+"&& b) {\n"
+"}",
+format("if (a\n"
+   "&& b) {\n"
+   "}",
+   Style, SC_ExpectComplete,
+   ExtraFormattingOptions::KeepLineBreaksForNonEmptyLines));
+
+  EXPECT_EQ("[]() {\n"
+"  foo(); }",
+format("[]() {\n"
+   "foo(); }",
+   Style, SC_ExpectComplete,
+   ExtraFormattingOptions::KeepLineBreaksForNonEmptyLines));
+}
+
 TEST_F(FormatTest, RecognizesBinaryOperatorKeywords) {
   verifyFormat("x = (a) and (b);");
   verifyFormat("x = (a) or (b);");
Index: tools/clang-format-ide/ClangFormatIDE.cpp
===
--- tools/clang-format-ide/ClangFormatIDE.cpp
+++ tools/clang-format-ide/ClangFormatIDE.cpp
@@ -0,0 +1,102 @@
+//===-- clang-format/ClangFormat.cpp - Clang format tool --===//
+//
+// 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
+//
+//===--===//
+///
+/// \file
+/// This file implements a clang-format tool that automatically formats
+/// (fragments of) C++ code.
+///
+//===--===//
+
+#include "../clang-format/ClangFormat.h"
+
+static cl::opt
+KeepLineBreaks("keep-line-breaks",
+   cl::desc("If set, keeps all existing line breaks"),
+   cl::cat(ClangFormatCategory));
+
+static void PrintVersion(raw_ostream ) {
+  OS << clang::getClangToolFullVersion("clang-format-ide") << '\n';
+}
+
+int main(int argc, const char **argv) {
+  llvm::InitLLVM X(argc, argv);
+
+  cl::HideUnrelatedOptions(ClangFormatCategory);
+
+  cl::SetVersionPrinter(PrintVersion);
+  cl::ParseCommandLineOptions(
+  argc, argv,
+  "A tool to format C/C++/Java/JavaScript/Objective-C/Protobuf/C# code.\n\n"
+  "If no arguments are specified, it formats the code from standard input\n"
+  "and writes the result to the standard output.\n"
+  "If s are given, it reformats the files. If -i is specified\n"
+  "together with s, the files are edited in-place. Otherwise, the\n"
+  "result is written to the standard output.\n");
+
+  if (Help) {
+cl::PrintHelpMessage();
+return 0;
+  }
+
+  if (DumpConfig) {
+StringRef FileName;
+std::unique_ptr Code;
+if (FileNames.empty()) {
+  // We can't read the code