[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

Oh, and please rename the patch before landing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72326

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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[Marek Kurdej via Phabricator via cfe-commits]

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

LGTM but please wait for the green light from other involved reviewers.
Also, as noted in a comment, I'd like to see unrelated changes in a different 
patch (unless you convince me that it's overly complicated or too much 
dependent on this patch).




Comment at: clang/lib/Format/Format.cpp:3298
+// Search for parent configs starting from the parent directory of
+// ConfigFile
+FileName = ConfigFile;

Here and elsewhere: please end the comments with a full stop.



Comment at: clang/unittests/Format/FormatTest.cpp:21483
 
-  // Test 9.1: overwriting a file style, when parent no file exists with no
+  // Test 9.1.1: overwriting a file style, when parent no file exists with no
   // fallback style

?



Comment at: clang/unittests/Format/FormatTest.cpp:21499
 
+  // Test 9.1.2: propagate more than one level with no parent file
+  ASSERT_TRUE(FS.addFile("/e/sub/sub/code.cpp", 0,

Can we split changes unrelated to the specific configuration file 
(fixing/testing the inherited config) to another patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72326

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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[Zhao Wei Liew via Phabricator via cfe-commits]

zwliew added inline comments.



Comment at: clang/lib/Format/Format.cpp:3393
   if (!ChildFormatTextToApply.empty()) {
 assert(ChildFormatTextToApply.size() == 1);
 

HazardyKnusperkeks wrote:
> zwliew wrote:
> > Is there a reason for this to be limited to 1? I came across this case 
> > during testing, but I can't think of why this should be so.
> See comment on D93844.
I've added a new test case (9.1.2) to illustrate the test failure, and also 
fixed the code to fix the test case.



Comment at: clang/lib/Format/Format.cpp:3276
+  // Check for explicit config filename
+  if (StyleName.startswith_insensitive("file:")) {
+auto StyleNameFile = StyleName.substr(5);

zwliew wrote:
> HazardyKnusperkeks wrote:
> > zwliew wrote:
> > > Following my code suggestion above, I think this code block could be 
> > > refactored to this: 
> > > ```
> > > // Check for explicit config filename
> > > if (StyleName.startswith_insensitive("file:")) {
> > > auto StyleFileName = StyleName.substr(5);
> > > if (auto ec = loadConfigFile(StyleFileName, FS, &Style)) {
> > > return make_string_error(ec.message());
> > > }
> > > LLVM_DEBUG(llvm::dbgs() << "Using configuration file " << 
> > > StyleFileName << '\n');
> > > return Style;
> > > }
> > > ```
> > > 
> > > What do you think?
> > > 
> > > Also, on another note, I feel like the `-style` option is too overloaded. 
> > > Would it be better to use a separate option like `-style-file` instead?
> > You can certainly refactor code.
> > 
> > What happens with `-style="Google" -style-file="Foo/Bar"` is it different 
> > from `-style-file="Foo/Bar" -style="Google"`?
> > I do not perse disagree, but I think it makes stuff clearer if there is 
> > only one option.
> Fair enough. Some ideas off the top of my head that resolve the ambiguity 
> sound needlessly complicated. Thanks!
I've refactored the code in the latest revision (under 
`loadAndParseConfigFile()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72326

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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[Zhao Wei Liew via Phabricator via cfe-commits]

zwliew updated this revision to Diff 396458.
zwliew added a comment.

Support inheritance in a chain of more than 1 parents

I made the following changes:

1. Refactor the code for loading and parsing configs into a separate function

2. Add a new test case (9.1.2) to test the case mentioned in 
https://reviews.llvm.org/D93844#inline-1112285.

3. Fix the test case failure for the newly added test 9.1.2.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72326

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

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -21480,7 +21480,7 @@
   ASSERT_TRUE((bool)StyleTd);
   ASSERT_EQ(*StyleTd, getLLVMStyle(FormatStyle::LK_TableGen));
 
-  // Test 9.1: overwriting a file style, when parent no file exists with no
+  // Test 9.1.1: overwriting a file style, when parent no file exists with no
   // fallback style
   ASSERT_TRUE(FS.addFile(
   "/e/sub/.clang-format", 0,
@@ -21496,6 +21496,25 @@
 return Style;
   }());
 
+  // Test 9.1.2: propagate more than one level with no parent file
+  ASSERT_TRUE(FS.addFile("/e/sub/sub/code.cpp", 0,
+ llvm::MemoryBuffer::getMemBuffer("int i;")));
+  ASSERT_TRUE(FS.addFile("/e/sub/sub/.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer(
+ "BasedOnStyle: InheritParentConfig\n"
+ "WhitespaceSensitiveMacros: ['FOO', 'BAR']")));
+  std::vector NonDefaultWhiteSpaceMacros{"FOO", "BAR"};
+
+  ASSERT_NE(Style9->WhitespaceSensitiveMacros, NonDefaultWhiteSpaceMacros);
+  Style9 = getStyle("file", "/e/sub/sub/code.cpp", "none", "", &FS);
+  ASSERT_TRUE(static_cast(Style9));
+  ASSERT_EQ(*Style9, [&NonDefaultWhiteSpaceMacros] {
+auto Style = getNoStyle();
+Style.ColumnLimit = 20;
+Style.WhitespaceSensitiveMacros = NonDefaultWhiteSpaceMacros;
+return Style;
+  }());
+
   // Test 9.2: with LLVM fallback style
   Style9 = getStyle("file", "/e/sub/code.cpp", "LLVM", "", &FS);
   ASSERT_TRUE(static_cast(Style9));
@@ -21519,15 +21538,7 @@
 return Style;
   }());
 
-  // Test 9.4: propagate more than one level
-  ASSERT_TRUE(FS.addFile("/e/sub/sub/code.cpp", 0,
- llvm::MemoryBuffer::getMemBuffer("int i;")));
-  ASSERT_TRUE(FS.addFile("/e/sub/sub/.clang-format", 0,
- llvm::MemoryBuffer::getMemBuffer(
- "BasedOnStyle: InheritParentConfig\n"
- "WhitespaceSensitiveMacros: ['FOO', 'BAR']")));
-  std::vector NonDefaultWhiteSpaceMacros{"FOO", "BAR"};
-
+  // Test 9.4: propagate more than one level with a parent file
   const auto SubSubStyle = [&NonDefaultWhiteSpaceMacros] {
 auto Style = getGoogleStyle();
 Style.ColumnLimit = 20;
@@ -21584,6 +21595,70 @@
 Style.IndentWidth = 7;
 return Style;
   }());
+
+  // Test 9.9: use inheritance from a specific config file
+  Style9 = getStyle("file:/e/sub/sub/.clang-format", "/e/sub/sub/code.cpp",
+"none", "", &FS);
+  ASSERT_TRUE(static_cast(Style9));
+  ASSERT_EQ(*Style9, SubSubStyle);
+}
+
+TEST(FormatStyle, GetStyleFromExternalFile) {
+  llvm::vfs::InMemoryFileSystem FS;
+  // Explicit format file in parent directory.
+  ASSERT_TRUE(
+  FS.addFile("/e/.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: LLVM")));
+  ASSERT_TRUE(
+  FS.addFile("/e/explicit.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
+  ASSERT_TRUE(FS.addFile("/e/sub/sub/sub/test.cpp", 0,
+ llvm::MemoryBuffer::getMemBuffer("int i;")));
+  auto Style = getStyle("file:/e/explicit.clang-format",
+"/e/sub/sub/sub/test.cpp", "LLVM", "", &FS);
+  ASSERT_TRUE(static_cast(Style));
+  ASSERT_EQ(*Style, getGoogleStyle());
+
+  // Relative path to a format file
+  ASSERT_TRUE(
+  FS.addFile("../../e/explicit.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
+  Style = getStyle("file:../../e/explicit.clang-format",
+   "/e/sub/sub/sub/test.cpp", "LLVM", "", &FS);
+  ASSERT_TRUE(static_cast(Style));
+  ASSERT_EQ(*Style, getGoogleStyle());
+
+  // Missing explicit format file
+  Style = getStyle("file:/e/missing.clang-format", "/e/sub/sub/sub/test.cpp",
+   "LLVM", "", &FS);
+  ASSERT_FALSE(static_cast(Style));
+  llvm::consumeError(Style.takeError());
+
+  // Format file from the filesystem
+  SmallString<128> FormatFilePath;
+  std::error_code ECF = llvm::sys::fs::createTemporaryFile(
+

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[Zhao Wei Liew via Phabricator via cfe-commits]

zwliew added a comment.

In D72326#3211701 , 
@HazardyKnusperkeks wrote:

> Do you plan to refactor something for this review, or do you think you are 
> done? Then I will look at it again as a whole.

I'm going to try refactoring the code for loading and parsing the config file 
into a separate function. I'll update the diff in no longer than a day.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72326

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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

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

HazardyKnusperkeks added a comment.

Do you plan to refactor something for this review, or do you think you are 
done? Then I will look at it again as a whole.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72326

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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[Zhao Wei Liew via Phabricator via cfe-commits]

zwliew added inline comments.



Comment at: clang/lib/Format/Format.cpp:3276
+  // Check for explicit config filename
+  if (StyleName.startswith_insensitive("file:")) {
+auto StyleNameFile = StyleName.substr(5);

HazardyKnusperkeks wrote:
> zwliew wrote:
> > Following my code suggestion above, I think this code block could be 
> > refactored to this: 
> > ```
> > // Check for explicit config filename
> > if (StyleName.startswith_insensitive("file:")) {
> > auto StyleFileName = StyleName.substr(5);
> > if (auto ec = loadConfigFile(StyleFileName, FS, &Style)) {
> > return make_string_error(ec.message());
> > }
> > LLVM_DEBUG(llvm::dbgs() << "Using configuration file " << StyleFileName 
> > << '\n');
> > return Style;
> > }
> > ```
> > 
> > What do you think?
> > 
> > Also, on another note, I feel like the `-style` option is too overloaded. 
> > Would it be better to use a separate option like `-style-file` instead?
> You can certainly refactor code.
> 
> What happens with `-style="Google" -style-file="Foo/Bar"` is it different 
> from `-style-file="Foo/Bar" -style="Google"`?
> I do not perse disagree, but I think it makes stuff clearer if there is only 
> one option.
Fair enough. Some ideas off the top of my head that resolve the ambiguity sound 
needlessly complicated. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72326

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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

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

HazardyKnusperkeks added inline comments.



Comment at: clang/lib/Format/Format.cpp:3393
   if (!ChildFormatTextToApply.empty()) {
 assert(ChildFormatTextToApply.size() == 1);
 

zwliew wrote:
> Is there a reason for this to be limited to 1? I came across this case during 
> testing, but I can't think of why this should be so.
See comment on D93844.



Comment at: clang/lib/Format/Format.cpp:3276
+  // Check for explicit config filename
+  if (StyleName.startswith_insensitive("file:")) {
+auto StyleNameFile = StyleName.substr(5);

zwliew wrote:
> Following my code suggestion above, I think this code block could be 
> refactored to this: 
> ```
> // Check for explicit config filename
> if (StyleName.startswith_insensitive("file:")) {
> auto StyleFileName = StyleName.substr(5);
> if (auto ec = loadConfigFile(StyleFileName, FS, &Style)) {
> return make_string_error(ec.message());
> }
> LLVM_DEBUG(llvm::dbgs() << "Using configuration file " << StyleFileName 
> << '\n');
> return Style;
> }
> ```
> 
> What do you think?
> 
> Also, on another note, I feel like the `-style` option is too overloaded. 
> Would it be better to use a separate option like `-style-file` instead?
You can certainly refactor code.

What happens with `-style="Google" -style-file="Foo/Bar"` is it different from 
`-style-file="Foo/Bar" -style="Google"`?
I do not perse disagree, but I think it makes stuff clearer if there is only 
one option.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72326

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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[Zhao Wei Liew via Phabricator via cfe-commits]

zwliew updated this revision to Diff 396274.
zwliew added a comment.

Add a test for inheritance from parent config with base


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72326

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

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -21584,6 +21584,70 @@
 Style.IndentWidth = 7;
 return Style;
   }());
+
+  // Test 9.9: use inheritance from a specific config file
+  Style9 = getStyle("file:/e/sub/sub/.clang-format", "/e/sub/sub/code.cpp",
+"none", "", &FS);
+  ASSERT_TRUE(static_cast(Style9));
+  ASSERT_EQ(*Style9, SubSubStyle);
+}
+
+TEST(FormatStyle, GetStyleFromExternalFile) {
+  llvm::vfs::InMemoryFileSystem FS;
+  // Explicit format file in parent directory.
+  ASSERT_TRUE(
+  FS.addFile("/e/.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: LLVM")));
+  ASSERT_TRUE(
+  FS.addFile("/e/explicit.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
+  ASSERT_TRUE(FS.addFile("/e/sub/sub/sub/test.cpp", 0,
+ llvm::MemoryBuffer::getMemBuffer("int i;")));
+  auto Style = getStyle("file:/e/explicit.clang-format",
+"/e/sub/sub/sub/test.cpp", "LLVM", "", &FS);
+  ASSERT_TRUE(static_cast(Style));
+  ASSERT_EQ(*Style, getGoogleStyle());
+
+  // Relative path to a format file
+  ASSERT_TRUE(
+  FS.addFile("../../e/explicit.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
+  Style = getStyle("file:../../e/explicit.clang-format",
+   "/e/sub/sub/sub/test.cpp", "LLVM", "", &FS);
+  ASSERT_TRUE(static_cast(Style));
+  ASSERT_EQ(*Style, getGoogleStyle());
+
+  // Missing explicit format file
+  Style = getStyle("file:/e/missing.clang-format", "/e/sub/sub/sub/test.cpp",
+   "LLVM", "", &FS);
+  ASSERT_FALSE(static_cast(Style));
+  llvm::consumeError(Style.takeError());
+
+  // Format file from the filesystem
+  SmallString<128> FormatFilePath;
+  std::error_code ECF = llvm::sys::fs::createTemporaryFile(
+  "FormatFileTest", "tpl", FormatFilePath);
+  EXPECT_FALSE((bool)ECF);
+  llvm::raw_fd_ostream FormatFileTest(FormatFilePath, ECF);
+  EXPECT_FALSE((bool)ECF);
+  FormatFileTest << "BasedOnStyle: Google\n";
+  FormatFileTest.close();
+
+  SmallString<128> TestFilePath;
+  std::error_code ECT =
+  llvm::sys::fs::createTemporaryFile("CodeFileTest", "cc", TestFilePath);
+  EXPECT_FALSE((bool)ECT);
+  llvm::raw_fd_ostream CodeFileTest(TestFilePath, ECT);
+  CodeFileTest << "int i;\n";
+  CodeFileTest.close();
+
+  std::string format_file_arg = std::string("file:") + FormatFilePath.c_str();
+  Style = getStyle(format_file_arg, TestFilePath, "LLVM", "", nullptr);
+
+  llvm::sys::fs::remove(FormatFilePath.c_str());
+  llvm::sys::fs::remove(TestFilePath.c_str());
+  ASSERT_TRUE(static_cast(Style));
+  ASSERT_EQ(*Style, getGoogleStyle());
 }
 
 TEST_F(ReplacementTest, FormatCodeAfterReplacements) {
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -3181,6 +3181,8 @@
 ".clang-format file located in one of the parent\n"
 "directories of the source file (or current\n"
 "directory for stdin).\n"
+"Use -style=file: to explicitly specify"
+"the configuration file.\n"
 "Use -style=\"{key: value, ...}\" to set specific\n"
 "parameters, e.g.:\n"
 "  -style=\"{BasedOnStyle: llvm, IndentWidth: 8}\"";
@@ -3263,6 +3265,31 @@
   return Style;
   }
 
+  // User provided clang-format file using -style=file:path/to/format/file
+  // Check for explicit config filename
+  if (!Style.InheritsParentConfig &&
+  StyleName.startswith_insensitive("file:")) {
+auto ConfigFile = StyleName.substr(5);
+llvm::ErrorOr> Text =
+FS->getBufferForFile(ConfigFile.str());
+if (auto EC = Text.getError())
+  return make_string_error(EC.message());
+if (auto EC = parseConfiguration(*Text.get(), &Style, AllowUnknownOptions))
+  return make_string_error("Error reading " + ConfigFile + ": " +
+   EC.message());
+
+LLVM_DEBUG(llvm::dbgs()
+   << "Using configuration file " << ConfigFile << "\n");
+
+if (!Style.InheritsParentConfig)
+  return Style;
+
+// Search for parent configs starting from the parent directory of
+// ConfigFile
+FileName = ConfigFile;
+ChildFormatTextToApply.emplace_back(std::move(*Text));
+  }
+
   // If the

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[Zhao Wei Liew via Phabricator via cfe-commits]

zwliew added inline comments.



Comment at: clang/lib/Format/Format.cpp:3393
   if (!ChildFormatTextToApply.empty()) {
 assert(ChildFormatTextToApply.size() == 1);
 

Is there a reason for this to be limited to 1? I came across this case during 
testing, but I can't think of why this should be so.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72326

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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[Zhao Wei Liew via Phabricator via cfe-commits]

zwliew updated this revision to Diff 396273.
zwliew added a comment.

Support inheritance from parent configs via `BasedOnStyle: InheritParentConfig`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72326

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

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -21586,6 +21586,64 @@
   }());
 }
 
+TEST(FormatStyle, GetStyleFromExternalFile) {
+  llvm::vfs::InMemoryFileSystem FS;
+  // Explicit format file in parent directory.
+  ASSERT_TRUE(
+  FS.addFile("/e/.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: LLVM")));
+  ASSERT_TRUE(
+  FS.addFile("/e/explicit.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
+  ASSERT_TRUE(FS.addFile("/e/sub/sub/sub/test.cpp", 0,
+ llvm::MemoryBuffer::getMemBuffer("int i;")));
+  auto Style = getStyle("file:/e/explicit.clang-format",
+"/e/sub/sub/sub/test.cpp", "LLVM", "", &FS);
+  ASSERT_TRUE((bool)Style);
+  ASSERT_EQ(*Style, getGoogleStyle());
+
+  // Relative pah to a format file
+  ASSERT_TRUE(
+  FS.addFile("../../e/explicit.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
+  Style = getStyle("file:../../e/explicit.clang-format",
+   "/e/sub/sub/sub/test.cpp", "LLVM", "", &FS);
+  ASSERT_TRUE((bool)Style);
+  ASSERT_EQ(*Style, getGoogleStyle());
+
+  // Missing explicit format file
+  Style = getStyle("file:/e/missing.clang-format", "/e/sub/sub/sub/test.cpp",
+   "LLVM", "", &FS);
+  ASSERT_FALSE((bool)Style);
+  llvm::consumeError(Style.takeError());
+
+  // Format file from the filesystem
+  SmallString<128> FormatFilePath;
+  std::error_code ECF = llvm::sys::fs::createTemporaryFile(
+  "FormatFileTest", "tpl", FormatFilePath);
+  EXPECT_FALSE((bool)ECF);
+  llvm::raw_fd_ostream FormatFileTest(FormatFilePath, ECF);
+  EXPECT_FALSE((bool)ECF);
+  FormatFileTest << "BasedOnStyle: Google\n";
+  FormatFileTest.close();
+
+  SmallString<128> TestFilePath;
+  std::error_code ECT =
+  llvm::sys::fs::createTemporaryFile("CodeFileTest", "cc", TestFilePath);
+  EXPECT_FALSE((bool)ECT);
+  llvm::raw_fd_ostream CodeFileTest(TestFilePath, ECT);
+  CodeFileTest << "int i;\n";
+  CodeFileTest.close();
+
+  std::string format_file_arg = std::string("file:") + FormatFilePath.c_str();
+  Style = getStyle(format_file_arg, TestFilePath, "LLVM", "", nullptr);
+
+  llvm::sys::fs::remove(FormatFilePath.c_str());
+  llvm::sys::fs::remove(TestFilePath.c_str());
+  ASSERT_TRUE((bool)Style);
+  ASSERT_EQ(*Style, getGoogleStyle());
+}
+
 TEST_F(ReplacementTest, FormatCodeAfterReplacements) {
   // Column limit is 20.
   std::string Code = "Type *a =\n"
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -3181,6 +3181,8 @@
 ".clang-format file located in one of the parent\n"
 "directories of the source file (or current\n"
 "directory for stdin).\n"
+"Use -style=file: to explicitly specify"
+"the configuration file.\n"
 "Use -style=\"{key: value, ...}\" to set specific\n"
 "parameters, e.g.:\n"
 "  -style=\"{BasedOnStyle: llvm, IndentWidth: 8}\"";
@@ -3263,6 +3265,31 @@
   return Style;
   }
 
+  // User provided clang-format file using -style=file:path/to/format/file
+  // Check for explicit config filename
+  if (!Style.InheritsParentConfig &&
+  StyleName.startswith_insensitive("file:")) {
+auto ConfigFile = StyleName.substr(5);
+llvm::ErrorOr> Text =
+FS->getBufferForFile(ConfigFile.str());
+if (auto EC = Text.getError())
+  return make_string_error(EC.message());
+if (auto EC = parseConfiguration(*Text.get(), &Style, AllowUnknownOptions))
+  return make_string_error("Error reading " + ConfigFile + ": " +
+   EC.message());
+
+LLVM_DEBUG(llvm::dbgs()
+   << "Using configuration file " << ConfigFile << "\n");
+
+if (!Style.InheritsParentConfig)
+  return Style;
+
+// Search for parent configs starting from the parent directory of
+// ConfigFile
+FileName = ConfigFile;
+ChildFormatTextToApply.emplace_back(std::move(*Text));
+  }
+
   // If the style inherits the parent configuration it is a command line
   // configuration, which wants to inherit, so we have to skip the check of the
   // StyleName.
Index: clang/include/clang/Format/Format.h
==

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay marked 4 inline comments as done.
MyDeveloperDay added a comment.

Please go ahead


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

https://reviews.llvm.org/D72326

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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[Zhao Wei Liew via Phabricator via cfe-commits]

zwliew added a comment.

On further thought, the logic for `loadConfigFile()` looks incomplete. It does 
not properly handle the `InheritParentConfig` argument for `BasedOnStyle`. In 
fact, `loadConfigFile()` should probably use the same logic as that for 
`-style=file`. I can look into making this change, and possibly refactoring the 
code to reduce code duplication.


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

https://reviews.llvm.org/D72326

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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[Zhao Wei Liew via Phabricator via cfe-commits]

zwliew added a comment.

Hi, I'd like to help to get this patch accepted and merged. I have a few 
suggestions/questions below, and I can help make any changes to the patch if 
needed!




Comment at: clang/docs/ClangFormatStyleOptions.rst:35
 
+When using ``-style=file:, :program:`clang-format` for 
+each input file will use the format file located at ``.

I think two backticks are missing here.



Comment at: clang/include/clang/Format/Format.h:4068
 /// directory if ``FileName`` is empty.
+/// * "file:" to explicitly specify the configuration file to use.
 ///

To be consistent with the `ClangFormatOptions.rst` docs, I think `configfile` 
should be changed to `format_file_path`.



Comment at: clang/lib/Format/Format.cpp:3166
 "directory for stdin).\n"
+"Use -style=file: to explicitly specify"
+"the configuration file.\n"

Similar to above, `configfile` should be changed to `format_file_path`.



Comment at: clang/lib/Format/Format.cpp:3219
+   llvm::vfs::FileSystem *FS,
+   bool *IsSuitable) {
+  FormatStyle Style = getLLVMStyle();

I think `LoadConfigFile` could be refactored to return `std::error_code` 
instead, similar to `parseConfiguration()`, like so:

```
// note I renamed LoadConfigFile to loadConfigFile to follow the existing 
function naming style.
std::error_code loadConfigFile(StringRef ConfigFile, llvm::vfs::FileSystem *FS, 
FormatStyle *Style) {
llvm::ErrorOr> Text = 
FS->getBufferForFile(ConfigFile.str());
if (auto ec = Text.getError()) {
return ec;
}
return parseConfiguration(Text.get()->getBuffer(), Style);
}
```

And the part in `getStyle()` would look like this:
```
// Check for explicit config filename
if (StyleName.startswith_insensitive("file:")) {
auto StyleFileName = StyleName.substr(5);
if (auto ec = loadConfigFile(StyleFileName, FS, &Style)) {
return make_string_error(ec.message());
}
LLVM_DEBUG(llvm::dbgs() << "Using configuration file " << StyleFileName << 
'\n');
return Style;
}
```

What do you think?



Comment at: clang/lib/Format/Format.cpp:3273
 
+  llvm::SmallVector FilesToLookFor;
+  // User provided clang-format file using -style=file:/path/to/format/file

HazardyKnusperkeks wrote:
> Why move that, it it's not used here?
Yup, I think this shouldn't be moved.



Comment at: clang/lib/Format/Format.cpp:3276
+  // Check for explicit config filename
+  if (StyleName.startswith_insensitive("file:")) {
+auto StyleNameFile = StyleName.substr(5);

Following my code suggestion above, I think this code block could be refactored 
to this: 
```
// Check for explicit config filename
if (StyleName.startswith_insensitive("file:")) {
auto StyleFileName = StyleName.substr(5);
if (auto ec = loadConfigFile(StyleFileName, FS, &Style)) {
return make_string_error(ec.message());
}
LLVM_DEBUG(llvm::dbgs() << "Using configuration file " << StyleFileName << 
'\n');
return Style;
}
```

What do you think?

Also, on another note, I feel like the `-style` option is too overloaded. Would 
it be better to use a separate option like `-style-file` instead?



Comment at: clang/lib/Format/Format.cpp:2693
+   llvm::vfs::FileSystem *FS,
+   bool *IsSuitable) {
+  FormatStyle Style = getLLVMStyle();

MyDeveloperDay wrote:
> pass Style in, don't keep assuming LLVM
I think it's still a good idea to pass Style into the function instead.


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

https://reviews.llvm.org/D72326

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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

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

HazardyKnusperkeks added inline comments.



Comment at: clang/lib/Format/Format.cpp:3231
+return Style;
+  } else if (ParserErrorCode != ParseError::Success) {
+return make_string_error("Error reading " + ConfigFile + ": " +

else after return

But I prefer a `switch` on `ParseErrorCode`.



Comment at: clang/lib/Format/Format.cpp:3273
 
+  llvm::SmallVector FilesToLookFor;
+  // User provided clang-format file using -style=file:/path/to/format/file

Why move that, it it's not used here?



Comment at: clang/lib/Format/Format.cpp:2743
+bool IsSuitable = true;
+auto Style = LoadConfigFile(StyleNameFile, FS, &IsSuitable);
+if (Style && !IsSuitable) {

MyDeveloperDay wrote:
> pass Style in and rename return value
Why did you not follow your own comment?


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

https://reviews.llvm.org/D72326

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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

Ping! Could we see this previously approved patch over the line I’d like to use 
it to build a regression suit of code and format files without the need for 
every test to be in its own directory


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

https://reviews.llvm.org/D72326

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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[Thibault North via Phabricator via cfe-commits]

tnorth added a comment.

Hi there,
Yes, sorry for the long silence, I ended up not having time to come back to 
this anymore for personal reasons. I also lack familiarity with the codebase 
and tests, and I couldn't invest more time into it; I had the impression that 
this last failing test would require major changes.
I'm happy if this ends up being merged. Thanks for your time and guidance again.


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

https://reviews.llvm.org/D72326

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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

The reason I have picked this us was because of:

https://twitter.com/bruxisma/status/1462987809879257101

This slightly annoys me because :

a) What they were talking about was in my view is disrespectful and inaccurate.
b) I thought we'd already landed this (which we had)

I went looking for this review which had previously been accepted and landed, 
but got reverted because it seemed to fail the tests

The original author and the original-original-author has both obviously moved 
on a dropped it and so it didn't get fixed.

I don't like wasting all that effort, especailly if we are going to get grief 
for it. So I rebased the review so we can land it again, (and checked both the 
unit tests and lit tests)

I hope we don't have to go around the houses on this too much.




Comment at: clang/lib/Format/Format.cpp:3274
+  llvm::SmallVector FilesToLookFor;
+  // User provided clang-format file using -style=file:/path/to/format/file
+  // Check for explicit config filename

HazardyKnusperkeks wrote:
> MyDeveloperDay wrote:
> > part of me wonders if this format should be
> > 
> > `file://` rather than `file:`
> > 
> > ```User provided clang-format file using 
> > -style=file:///path/to/format/file```
> > 
> > vs
> > 
> > ```User provided clang-format file using -style=file:/path/to/format/file```
> > 
> > This would leave the way open to other protocols http:// or https:// or 
> > anything else.
> Maybe, but I doubt we want http. And if we use `file://` a windows path 
> `D:\Path\` would not be valid, would it?
yes file://C:\Windows\Kernel32.dll is a valid file url.

but I on reflection I think `file:`  will be ok.


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

https://reviews.llvm.org/D72326

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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

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

HazardyKnusperkeks added inline comments.



Comment at: clang/lib/Format/Format.cpp:3274
+  llvm::SmallVector FilesToLookFor;
+  // User provided clang-format file using -style=file:/path/to/format/file
+  // Check for explicit config filename

MyDeveloperDay wrote:
> part of me wonders if this format should be
> 
> `file://` rather than `file:`
> 
> ```User provided clang-format file using -style=file:///path/to/format/file```
> 
> vs
> 
> ```User provided clang-format file using -style=file:/path/to/format/file```
> 
> This would leave the way open to other protocols http:// or https:// or 
> anything else.
Maybe, but I doubt we want http. And if we use `file://` a windows path 
`D:\Path\` would not be valid, would it?


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

https://reviews.llvm.org/D72326

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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/lib/Format/Format.cpp:3274
+  llvm::SmallVector FilesToLookFor;
+  // User provided clang-format file using -style=file:/path/to/format/file
+  // Check for explicit config filename

part of me wonders if this format should be

`file://` rather than `file:`

```User provided clang-format file using -style=file:///path/to/format/file```

vs

```User provided clang-format file using -style=file:/path/to/format/file```

This would leave the way open to other protocols http:// or https:// or 
anything else.


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

https://reviews.llvm.org/D72326

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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 389488.
MyDeveloperDay added a comment.

Rebasing previously landed but reverted commit


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

https://reviews.llvm.org/D72326

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

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -21343,6 +21343,64 @@
   }());
 }
 
+TEST(FormatStyle, GetStyleFromExteranlFile) {
+  llvm::vfs::InMemoryFileSystem FS;
+  // Explicit format file in parent directory.
+  ASSERT_TRUE(
+  FS.addFile("/e/.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: LLVM")));
+  ASSERT_TRUE(
+  FS.addFile("/e/explicit.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
+  ASSERT_TRUE(FS.addFile("/e/sub/sub/sub/test.cpp", 0,
+ llvm::MemoryBuffer::getMemBuffer("int i;")));
+  auto Style = getStyle("file:/e/explicit.clang-format",
+"/e/sub/sub/sub/test.cpp", "LLVM", "", &FS);
+  ASSERT_TRUE((bool)Style);
+  ASSERT_EQ(*Style, getGoogleStyle());
+
+  // Relative pah to a format file
+  ASSERT_TRUE(
+  FS.addFile("../../e/explicit.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
+  Style = getStyle("file:../../e/explicit.clang-format",
+   "/e/sub/sub/sub/test.cpp", "LLVM", "", &FS);
+  ASSERT_TRUE((bool)Style);
+  ASSERT_EQ(*Style, getGoogleStyle());
+
+  // Missing explicit format file
+  Style = getStyle("file:/e/missing.clang-format", "/e/sub/sub/sub/test.cpp",
+   "LLVM", "", &FS);
+  ASSERT_FALSE((bool)Style);
+  llvm::consumeError(Style.takeError());
+
+  // Format file from the filesystem
+  SmallString<128> FormatFilePath;
+  std::error_code ECF = llvm::sys::fs::createTemporaryFile(
+  "FormatFileTest", "tpl", FormatFilePath);
+  EXPECT_FALSE((bool)ECF);
+  llvm::raw_fd_ostream FormatFileTest(FormatFilePath, ECF);
+  EXPECT_FALSE((bool)ECF);
+  FormatFileTest << "BasedOnStyle: Google\n";
+  FormatFileTest.close();
+
+  SmallString<128> TestFilePath;
+  std::error_code ECT =
+  llvm::sys::fs::createTemporaryFile("CodeFileTest", "cc", TestFilePath);
+  EXPECT_FALSE((bool)ECT);
+  llvm::raw_fd_ostream CodeFileTest(TestFilePath, ECT);
+  CodeFileTest << "int i;\n";
+  CodeFileTest.close();
+
+  std::string format_file_arg = std::string("file:") + FormatFilePath.c_str();
+  Style = getStyle(format_file_arg, TestFilePath, "LLVM", "", nullptr);
+
+  llvm::sys::fs::remove(FormatFilePath.c_str());
+  llvm::sys::fs::remove(TestFilePath.c_str());
+  ASSERT_TRUE((bool)Style);
+  ASSERT_EQ(*Style, getGoogleStyle());
+}
+
 TEST_F(ReplacementTest, FormatCodeAfterReplacements) {
   // Column limit is 20.
   std::string Code = "Type *a =\n"
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -3163,6 +3163,8 @@
 ".clang-format file located in one of the parent\n"
 "directories of the source file (or current\n"
 "directory for stdin).\n"
+"Use -style=file: to explicitly specify"
+"the configuration file.\n"
 "Use -style=\"{key: value, ...}\" to set specific\n"
 "parameters, e.g.:\n"
 "  -style=\"{BasedOnStyle: llvm, IndentWidth: 8}\"";
@@ -3211,6 +3213,29 @@
   return GuessedLanguage;
 }
 
+/// Attempts to load a format file
+llvm::Expected LoadConfigFile(StringRef ConfigFile,
+   llvm::vfs::FileSystem *FS,
+   bool *IsSuitable) {
+  FormatStyle Style = getLLVMStyle();
+  *IsSuitable = false;
+
+  llvm::ErrorOr> Text =
+  FS->getBufferForFile(ConfigFile.str());
+  if (std::error_code EC = Text.getError())
+return make_string_error(EC.message());
+  std::error_code ParserErrorCode =
+  parseConfiguration(Text.get()->getBuffer(), &Style);
+  if (ParserErrorCode == ParseError::Unsuitable) {
+return Style;
+  } else if (ParserErrorCode != ParseError::Success) {
+return make_string_error("Error reading " + ConfigFile + ": " +
+ ParserErrorCode.message());
+  }
+  *IsSuitable = true;
+  return Style;
+}
+
 const char *DefaultFormatStyle = "file";
 
 const char *DefaultFallbackStyle = "LLVM";
@@ -3245,6 +3270,23 @@
   return Style;
   }
 
+  llvm::SmallVector FilesToLookFor;
+  // User provided clang-format file using -style=file:/path/to/format/file
+  // Check for explicit config filename
+  if (StyleName.startswith_insensitive("file:")) {
+auto StyleNameFile = StyleName.substr(5);
+bool IsSuitable

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

After rebasing these tests seem to work (on Windows)

  -- Testing: 24 tests, 16 workers --
  PASS: Clang :: Format/dry-run-alias.cpp (1 of 24)
  PASS: Clang :: Format/remove-duplicate-includes.cpp (2 of 24)
  PASS: Clang :: Format/disable-format.cpp (3 of 24)
  PASS: Clang :: Format/cursor.cpp (4 of 24)
  PASS: Clang :: Format/multiple-inputs.cpp (5 of 24)
  PASS: Clang :: Format/basic.cpp (6 of 24)
  PASS: Clang :: Format/incomplete.cpp (7 of 24)
  PASS: Clang :: Format/line-ranges.cpp (8 of 24)
  PASS: Clang :: Format/adjust-indent.cpp (9 of 24)
  UNSUPPORTED: Clang :: Format/style-on-command-line.cpp (10 of 24)
  PASS: Clang :: Format/ranges.cpp (11 of 24)
  PASS: Clang :: Format/access-modifiers.cpp (12 of 24)
  PASS: Clang :: Format/multiple-inputs-error.cpp (13 of 24)
  PASS: Clang :: Format/dump-config-objc.h (14 of 24)
  PASS: Clang :: Format/xmloutput.cpp (15 of 24)
  PASS: Clang :: Format/dump-config-cxx.h (16 of 24)
  PASS: Clang :: Format/multiple-inputs-inplace.cpp (17 of 24)
  PASS: Clang :: Format/error-config.cpp (18 of 24)
  PASS: Clang :: Format/dry-run.cpp (19 of 24)
  PASS: Clang :: Format/inplace.cpp (20 of 24)
  PASS: Clang :: Format/dump-config-list-override.cpp (21 of 24)
  PASS: Clang :: Format/disable-include-sorting.cpp (22 of 24)
  PASS: Clang :: Format/language-detection.cpp (23 of 24)
  PASS: Clang :: Format/verbose.cpp (24 of 24)

I'm going to Commandeer this revision and see if we can't get this relanded.


Repository:
  rC Clang

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

https://reviews.llvm.org/D72326

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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

@tnorth Are you planning on working on this or do you mind if one of us fixes 
the issues enough to get it over the line. (this was previously accepted and 
landed but the unit tests fail, for what looks like a fairly minor issue)


Repository:
  rC Clang

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

https://reviews.llvm.org/D72326

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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

ping @tnorth are you planning on fixing this as its currently reverted.


Repository:
  rC Clang

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

https://reviews.llvm.org/D72326



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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay requested changes to this revision.
MyDeveloperDay added inline comments.
This revision now requires changes to proceed.



Comment at: clang/lib/Format/Format.cpp:2693
+   llvm::vfs::FileSystem *FS,
+   bool *IsSuitable) {
+  FormatStyle Style = getLLVMStyle();

pass Style in, don't keep assuming LLVM



Comment at: clang/lib/Format/Format.cpp:2694
+   bool *IsSuitable) {
+  FormatStyle Style = getLLVMStyle();
+  *IsSuitable = false;

You cannot keep assuming the style is LLVM, Style needs to be passed in

```
/// Attempts to load a format file
llvm::Expected LoadConfigFile(StringRef ConfigFile,
   llvm::vfs::FileSystem *FS,
   bool *IsSuitable,
   FormatStyle Style) {
  *IsSuitable = false;

  llvm::ErrorOr> Text =
  FS->getBufferForFile(ConfigFile.str());
  if (std::error_code EC = Text.getError())
return make_string_error(EC.message());
  std::error_code ParserErrorCode =
  parseConfiguration(Text.get()->getBuffer(), &Style);
  if (ParserErrorCode == ParseError::Unsuitable) {
return Style;
  } else if (ParserErrorCode != ParseError::Success) {
return make_string_error("Error reading " + ConfigFile + ": " +
 ParserErrorCode.message());
  }
  *IsSuitable = true;
  return Style;
}

Now your code will read

```
  // User provided clang-format file using -style=file:/path/to/format/file
  // Check for explicit config filename
  if (StyleName.startswith_lower("file:")) {
auto StyleNameFile = StyleName.substr(5);
bool IsSuitable = true;
auto FileStyle = LoadConfigFile(StyleNameFile, FS, &IsSuitable, Style);
if (FileStyle && !IsSuitable) {
  return make_string_error("Configuration file(s) do(es) not support " +
   getLanguageName((*FileStyle).Language) + ": " +
   StyleNameFile);
}
return FileStyle;
  }

```


And then in the loop of files you pass the `Style` in too
if (auto ConfigStyle =
LoadConfigFile(ConfigFile, FS, &IsSuitable, Style)) {
```







Comment at: clang/lib/Format/Format.cpp:2743
+bool IsSuitable = true;
+auto Style = LoadConfigFile(StyleNameFile, FS, &IsSuitable);
+if (Style && !IsSuitable) {

pass Style in and rename return value



Comment at: clang/lib/Format/Format.cpp:2763
-LLVM_DEBUG(llvm::dbgs()
-   << "Using configuration file " << ConfigFile << "\n");
-return Style;

We should probably keep something like this so we know which config file we are 
loading



Comment at: clang/lib/Format/Format.cpp:2785
+bool IsSuitable;
+if (auto ConfigStyle = LoadConfigFile(ConfigFile, FS, &IsSuitable)) {
+  if (!IsSuitable) {

Pass current Style in


Repository:
  rC Clang

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

https://reviews.llvm.org/D72326



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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

These tests still fail

running the following in the build directory (if your build directory is 
side-by-side with the llvm-project directory):

c:/Python37/python ./bin/llvm-lit.py -v ./tools/clang/test/Format

  $ ./run_format_lit_tests.sh
  llvm-lit.py: 
C:\cygwin64\buildareas\clang\build\bin\..\..\llvm-project\llvm\utils\lit\lit\llvm\config.py:343:
 note: using clang: c:\cygwin64\buildareas\clang\build\bin\clang.exe
  -- Testing: 21 tests, 12 workers --
  PASS: Clang :: Format/cursor.cpp (1 of 21)
  FAIL: Clang :: Format/dump-config-objc.h (2 of 21)
   TEST 'Clang :: Format/dump-config-objc.h' FAILED 

  Script:
  --
  : 'RUN: at line 1';   c:\cygwin64\buildareas\clang\build\bin\clang-format.exe 
-dump-config 
C:\cygwin64\buildareas\clang\llvm-project\clang\test\Format\dump-config-objc.h 
| c:\cygwin64\buildareas\clang\build\bin\filecheck.exe 
C:\cygwin64\buildareas\clang\llvm-project\clang\test\Format\dump-config-objc.h
  --
  Exit Code: 1
  
  Command Output (stdout):
  --
  $ ":" "RUN: at line 1"
  $ "c:\cygwin64\buildareas\clang\build\bin\clang-format.exe" "-dump-config" 
"C:\cygwin64\buildareas\clang\llvm-project\clang\test\Format\dump-config-objc.h"
  $ "c:\cygwin64\buildareas\clang\build\bin\filecheck.exe" 
"C:\cygwin64\buildareas\clang\llvm-project\clang\test\Format\dump-config-objc.h"
  # command stderr:
  
C:\cygwin64\buildareas\clang\llvm-project\clang\test\Format\dump-config-objc.h:3:11:
 error: CHECK: expected string not found in input
  // CHECK: Language: ObjC
^
  :1:1: note: scanning from here
  ---
  ^
  :2:1: note: possible intended match here
  Language: Cpp
  ^
  
  error: command failed with exit status: 1
  
  --
  
  


Repository:
  rC Clang

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

https://reviews.llvm.org/D72326



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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

Were you able to rerun these tests?


Repository:
  rC Clang

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

https://reviews.llvm.org/D72326



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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[Thibault North via Phabricator via cfe-commits]

tnorth updated this revision to Diff 258029.

Repository:
  rC Clang

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

https://reviews.llvm.org/D72326

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

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -15204,6 +15204,61 @@
   auto StyleTd = getStyle("file", "x.td", "llvm", "", &FS);
   ASSERT_TRUE((bool)StyleTd);
   ASSERT_EQ(*StyleTd, getLLVMStyle(FormatStyle::LK_TableGen));
+
+  // Test 9: explicit format file in parent directory.
+  ASSERT_TRUE(
+  FS.addFile("/e/.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: LLVM")));
+  ASSERT_TRUE(
+  FS.addFile("/e/explicit.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
+  ASSERT_TRUE(FS.addFile("/e/sub/sub/sub/test.cpp", 0,
+ llvm::MemoryBuffer::getMemBuffer("int i;")));
+  auto Style8 = getStyle("file:/e/explicit.clang-format",
+ "/e/sub/sub/sub/test.cpp", "LLVM", "", &FS);
+  ASSERT_TRUE((bool)Style8);
+  ASSERT_EQ(*Style8, getGoogleStyle());
+
+  // Test 10: relative pah to a format file
+  ASSERT_TRUE(
+  FS.addFile("../../e/explicit.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
+  auto Style9 = getStyle("file:../../e/explicit.clang-format",
+ "/e/sub/sub/sub/test.cpp", "LLVM", "", &FS);
+  ASSERT_TRUE((bool)Style9);
+  ASSERT_EQ(*Style9, getGoogleStyle());
+
+  // Test 11: missing explicit format file
+  auto Style10 = getStyle("file:/e/missing.clang-format",
+  "/e/sub/sub/sub/test.cpp", "LLVM", "", &FS);
+  ASSERT_FALSE((bool)Style10);
+  llvm::consumeError(Style10.takeError());
+
+  // Test 12: format file from the filesystem
+  SmallString<128> FormatFilePath;
+  std::error_code ECF = llvm::sys::fs::createTemporaryFile(
+  "FormatFileTest", "tpl", FormatFilePath);
+  EXPECT_FALSE((bool)ECF);
+  llvm::raw_fd_ostream FormatFileTest(FormatFilePath, ECF);
+  EXPECT_FALSE((bool)ECF);
+  FormatFileTest << "BasedOnStyle: Google\n";
+  FormatFileTest.close();
+
+  SmallString<128> TestFilePath;
+  std::error_code ECT =
+  llvm::sys::fs::createTemporaryFile("CodeFileTest", "cc", TestFilePath);
+  EXPECT_FALSE((bool)ECT);
+  llvm::raw_fd_ostream CodeFileTest(TestFilePath, ECT);
+  CodeFileTest << "int i;\n";
+  CodeFileTest.close();
+
+  std::string format_file_arg = std::string("file:") + FormatFilePath.c_str();
+  auto Style11 = getStyle(format_file_arg, TestFilePath, "LLVM", "", nullptr);
+
+  llvm::sys::fs::remove(FormatFilePath.c_str());
+  llvm::sys::fs::remove(TestFilePath.c_str());
+  ASSERT_TRUE((bool)Style11);
+  ASSERT_EQ(*Style11, getGoogleStyle());
 }
 
 TEST_F(ReplacementTest, FormatCodeAfterReplacements) {
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -2640,6 +2640,8 @@
 ".clang-format file located in one of the parent\n"
 "directories of the source file (or current\n"
 "directory for stdin).\n"
+"Use -style=file: to explicitly specify"
+"the configuration file.\n"
 "Use -style=\"{key: value, ...}\" to set specific\n"
 "parameters, e.g.:\n"
 "  -style=\"{BasedOnStyle: llvm, IndentWidth: 8}\"";
@@ -2685,6 +2687,29 @@
   return GuessedLanguage;
 }
 
+/// Attempts to load a format file
+llvm::Expected LoadConfigFile(StringRef ConfigFile,
+   llvm::vfs::FileSystem *FS,
+   bool *IsSuitable) {
+  FormatStyle Style = getLLVMStyle();
+  *IsSuitable = false;
+
+  llvm::ErrorOr> Text =
+  FS->getBufferForFile(ConfigFile.str());
+  if (std::error_code EC = Text.getError())
+return make_string_error(EC.message());
+  std::error_code ParserErrorCode =
+  parseConfiguration(Text.get()->getBuffer(), &Style);
+  if (ParserErrorCode == ParseError::Unsuitable) {
+return Style;
+  } else if (ParserErrorCode != ParseError::Success) {
+return make_string_error("Error reading " + ConfigFile + ": " +
+ ParserErrorCode.message());
+  }
+  *IsSuitable = true;
+  return Style;
+}
+
 const char *DefaultFormatStyle = "file";
 
 const char *DefaultFallbackStyle = "LLVM";
@@ -2709,6 +2734,21 @@
 return Style;
   }
 
+  llvm::SmallVector FilesToLookFor;
+  // User provided clang-format file using -style=file:/path/to/format/file
+  // Check for explicit config filename
+  if (StyleName.startswith_lower("file:")) {
+auto StyleNameFile = StyleName.substr(5);
+bool IsS

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/lib/Format/Format.cpp:2704
+  if (ParserErrorCode == ParseError::Unsuitable) {
+*IsSuitable = false;
+return Style;

I think this isn't needed won't it already be false from line @2695


Repository:
  rC Clang

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

https://reviews.llvm.org/D72326



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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[Thibault North via Phabricator via cfe-commits]

tnorth updated this revision to Diff 257970.
tnorth added a comment.

Sorry.


Repository:
  rC Clang

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

https://reviews.llvm.org/D72326

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

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -15204,6 +15204,61 @@
   auto StyleTd = getStyle("file", "x.td", "llvm", "", &FS);
   ASSERT_TRUE((bool)StyleTd);
   ASSERT_EQ(*StyleTd, getLLVMStyle(FormatStyle::LK_TableGen));
+
+  // Test 9: explicit format file in parent directory.
+  ASSERT_TRUE(
+  FS.addFile("/e/.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: LLVM")));
+  ASSERT_TRUE(
+  FS.addFile("/e/explicit.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
+  ASSERT_TRUE(FS.addFile("/e/sub/sub/sub/test.cpp", 0,
+ llvm::MemoryBuffer::getMemBuffer("int i;")));
+  auto Style8 = getStyle("file:/e/explicit.clang-format",
+ "/e/sub/sub/sub/test.cpp", "LLVM", "", &FS);
+  ASSERT_TRUE((bool)Style8);
+  ASSERT_EQ(*Style8, getGoogleStyle());
+
+  // Test 10: relative pah to a format file
+  ASSERT_TRUE(
+  FS.addFile("../../e/explicit.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
+  auto Style9 = getStyle("file:../../e/explicit.clang-format",
+ "/e/sub/sub/sub/test.cpp", "LLVM", "", &FS);
+  ASSERT_TRUE((bool)Style9);
+  ASSERT_EQ(*Style9, getGoogleStyle());
+
+  // Test 11: missing explicit format file
+  auto Style10 = getStyle("file:/e/missing.clang-format",
+  "/e/sub/sub/sub/test.cpp", "LLVM", "", &FS);
+  ASSERT_FALSE((bool)Style10);
+  llvm::consumeError(Style10.takeError());
+
+  // Test 12: format file from the filesystem
+  SmallString<128> FormatFilePath;
+  std::error_code ECF = llvm::sys::fs::createTemporaryFile(
+  "FormatFileTest", "tpl", FormatFilePath);
+  EXPECT_FALSE((bool)ECF);
+  llvm::raw_fd_ostream FormatFileTest(FormatFilePath, ECF);
+  EXPECT_FALSE((bool)ECF);
+  FormatFileTest << "BasedOnStyle: Google\n";
+  FormatFileTest.close();
+
+  SmallString<128> TestFilePath;
+  std::error_code ECT =
+  llvm::sys::fs::createTemporaryFile("CodeFileTest", "cc", TestFilePath);
+  EXPECT_FALSE((bool)ECT);
+  llvm::raw_fd_ostream CodeFileTest(TestFilePath, ECT);
+  CodeFileTest << "int i;\n";
+  CodeFileTest.close();
+
+  std::string format_file_arg = std::string("file:") + FormatFilePath.c_str();
+  auto Style11 = getStyle(format_file_arg, TestFilePath, "LLVM", "", nullptr);
+
+  llvm::sys::fs::remove(FormatFilePath.c_str());
+  llvm::sys::fs::remove(TestFilePath.c_str());
+  ASSERT_TRUE((bool)Style11);
+  ASSERT_EQ(*Style11, getGoogleStyle());
 }
 
 TEST_F(ReplacementTest, FormatCodeAfterReplacements) {
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -2640,6 +2640,8 @@
 ".clang-format file located in one of the parent\n"
 "directories of the source file (or current\n"
 "directory for stdin).\n"
+"Use -style=file: to explicitly specify"
+"the configuration file.\n"
 "Use -style=\"{key: value, ...}\" to set specific\n"
 "parameters, e.g.:\n"
 "  -style=\"{BasedOnStyle: llvm, IndentWidth: 8}\"";
@@ -2685,6 +2687,30 @@
   return GuessedLanguage;
 }
 
+/// Attempts to load a format file
+llvm::Expected LoadConfigFile(StringRef ConfigFile,
+   llvm::vfs::FileSystem *FS,
+   bool *IsSuitable) {
+  FormatStyle Style = getLLVMStyle();
+  *IsSuitable = false;
+
+  llvm::ErrorOr> Text =
+  FS->getBufferForFile(ConfigFile.str());
+  if (std::error_code EC = Text.getError())
+return make_string_error(EC.message());
+  std::error_code ParserErrorCode =
+  parseConfiguration(Text.get()->getBuffer(), &Style);
+  if (ParserErrorCode == ParseError::Unsuitable) {
+*IsSuitable = false;
+return Style;
+  } else if (ParserErrorCode != ParseError::Success) {
+return make_string_error("Error reading " + ConfigFile + ": " +
+ ParserErrorCode.message());
+  }
+  *IsSuitable = true;
+  return Style;
+}
+
 const char *DefaultFormatStyle = "file";
 
 const char *DefaultFallbackStyle = "LLVM";
@@ -2709,6 +2735,21 @@
 return Style;
   }
 
+  llvm::SmallVector FilesToLookFor;
+  // User provided clang-format file using -style=file:/path/to/format/file
+  // Check for explicit config filename
+  if (StyleName.startswith_lower("file:")) {
+

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/lib/Format/Format.cpp:2700
+  if (std::error_code EC = Text.getError())
+return make_string_error(EC.message());
+  std::error_code ParserErrorCode =

if you return here IsSuitable will be true..



Comment at: clang/lib/Format/Format.cpp:2707
+  } else if (ParserErrorCode != ParseError::Success) {
+return make_string_error("Error reading " + ConfigFile + ": " +
+ ParserErrorCode.message());

again here you need to set IsSutable to false



Comment at: clang/lib/Format/Format.cpp:2709
+ ParserErrorCode.message());
+  }
+  return Style;

maybe its better just to set this to true here *IsSuitable = true; and set it 
to false on line @2695



Comment at: clang/lib/Format/Format.cpp:2792
+  } else {
+return *ConfigStyle;
   }

this could then mean you return with something wrong here..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72326



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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[Thibault North via Phabricator via cfe-commits]

tnorth added a comment.

Ping?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72326



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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[Thibault North via Phabricator via cfe-commits]

tnorth added a comment.

Ping @MyDeveloperDay  , any clue about what's going on here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72326



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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[Mitchell via Phabricator via cfe-commits]

mitchell-stellar reopened this revision.
mitchell-stellar added a comment.
This revision is now accepted and ready to land.

Reverted this commit due to an unexpected test failure:

   TEST 'Clang :: Format/dump-config-objc.h' FAILED 

  Script:
  --
  : 'RUN: at line 1';   /b/1/clang-x86_64-debian-fast/llvm.obj/bin/clang-format 
-dump-config 
/b/1/clang-x86_64-debian-fast/llvm.src/clang/test/Format/dump-config-objc.h | 
/b/1/clang-x86_64-debian-fast/llvm.obj/bin/FileCheck 
/b/1/clang-x86_64-debian-fast/llvm.src/clang/test/Format/dump-config-objc.h
  --
  Exit Code: 1
  
  Command Output (stderr):
  --
  
/b/1/clang-x86_64-debian-fast/llvm.src/clang/test/Format/dump-config-objc.h:3:11:
 error: CHECK: expected string not found in input
  // CHECK: Language: ObjC
^
  :1:1: note: scanning from here
  ---
  ^
  :2:1: note: possible intended match here
  Language: Cpp
  ^
  
  --
  
  

I don't know enough about this patch in order to determine what the issue is, 
or how to proceed further. Perhaps @MyDeveloperDay will chime in.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72326



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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[Mitchell via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG10b1a87ba35d: [clang-format] Add option to specify explicit 
config file Summary: This diff… (authored by mitchell-stellar).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72326

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

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -14912,6 +14912,61 @@
   auto StyleTd = getStyle("file", "x.td", "llvm", "", &FS);
   ASSERT_TRUE((bool)StyleTd);
   ASSERT_EQ(*StyleTd, getLLVMStyle(FormatStyle::LK_TableGen));
+
+  // Test 9: explicit format file in parent directory.
+  ASSERT_TRUE(
+  FS.addFile("/e/.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: LLVM")));
+  ASSERT_TRUE(
+  FS.addFile("/e/explicit.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
+  ASSERT_TRUE(FS.addFile("/e/sub/sub/sub/test.cpp", 0,
+ llvm::MemoryBuffer::getMemBuffer("int i;")));
+  auto Style8 = getStyle("file:/e/explicit.clang-format",
+ "/e/sub/sub/sub/test.cpp", "LLVM", "", &FS);
+  ASSERT_TRUE((bool)Style8);
+  ASSERT_EQ(*Style8, getGoogleStyle());
+
+  // Test 10: relative pah to a format file
+  ASSERT_TRUE(
+  FS.addFile("../../e/explicit.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
+  auto Style9 = getStyle("file:../../e/explicit.clang-format",
+ "/e/sub/sub/sub/test.cpp", "LLVM", "", &FS);
+  ASSERT_TRUE((bool)Style9);
+  ASSERT_EQ(*Style9, getGoogleStyle());
+
+  // Test 11: missing explicit format file
+  auto Style10 = getStyle("file:/e/missing.clang-format",
+  "/e/sub/sub/sub/test.cpp", "LLVM", "", &FS);
+  ASSERT_FALSE((bool)Style10);
+  llvm::consumeError(Style10.takeError());
+
+  // Test 12: format file from the filesystem
+  SmallString<128> FormatFilePath;
+  std::error_code ECF = llvm::sys::fs::createTemporaryFile(
+  "FormatFileTest", "tpl", FormatFilePath);
+  EXPECT_FALSE((bool)ECF);
+  llvm::raw_fd_ostream FormatFileTest(FormatFilePath, ECF);
+  EXPECT_FALSE((bool)ECF);
+  FormatFileTest << "BasedOnStyle: Google\n";
+  FormatFileTest.close();
+
+  SmallString<128> TestFilePath;
+  std::error_code ECT =
+  llvm::sys::fs::createTemporaryFile("CodeFileTest", "cc", TestFilePath);
+  EXPECT_FALSE((bool)ECT);
+  llvm::raw_fd_ostream CodeFileTest(TestFilePath, ECT);
+  CodeFileTest << "int i;\n";
+  CodeFileTest.close();
+
+  std::string format_file_arg = std::string("file:") + FormatFilePath.c_str();
+  auto Style11 = getStyle(format_file_arg, TestFilePath, "LLVM", "", nullptr);
+
+  llvm::sys::fs::remove(FormatFilePath.c_str());
+  llvm::sys::fs::remove(TestFilePath.c_str());
+  ASSERT_TRUE((bool)Style11);
+  ASSERT_EQ(*Style11, getGoogleStyle());
 }
 
 TEST_F(ReplacementTest, FormatCodeAfterReplacements) {
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -2640,6 +2640,8 @@
 ".clang-format file located in one of the parent\n"
 "directories of the source file (or current\n"
 "directory for stdin).\n"
+"Use -style=file: to explicitly specify"
+"the configuration file.\n"
 "Use -style=\"{key: value, ...}\" to set specific\n"
 "parameters, e.g.:\n"
 "  -style=\"{BasedOnStyle: llvm, IndentWidth: 8}\"";
@@ -2685,6 +2687,29 @@
   return GuessedLanguage;
 }
 
+/// Attempts to load a format file
+llvm::Expected LoadConfigFile(StringRef ConfigFile,
+   llvm::vfs::FileSystem *FS,
+   bool *IsSuitable) {
+  FormatStyle Style = getLLVMStyle();
+  *IsSuitable = true;
+
+  llvm::ErrorOr> Text =
+  FS->getBufferForFile(ConfigFile.str());
+  if (std::error_code EC = Text.getError())
+return make_string_error(EC.message());
+  std::error_code ParserErrorCode =
+  parseConfiguration(Text.get()->getBuffer(), &Style);
+  if (ParserErrorCode == ParseError::Unsuitable) {
+*IsSuitable = false;
+return Style;
+  } else if (ParserErrorCode != ParseError::Success) {
+return make_string_error("Error reading " + ConfigFile + ": " +
+ ParserErrorCode.message());
+  }
+  return Style;
+}
+
 const char *DefaultFormatStyle = "file";
 
 const char *DefaultFallbackStyle = "LLVM";
@@ -2709,6 +2734,21 @@
 return Style;
   }
 
+  llvm::SmallVector FilesToLookFor;
+  // User provided clang-format f

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[Thibault North via Phabricator via cfe-commits]

tnorth updated this revision to Diff 249616.
tnorth added a comment.

Rebased on master (6d5603e2 
). Some 
refactoring was indeed needed.


Repository:
  rC Clang

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

https://reviews.llvm.org/D72326

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

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -14912,6 +14912,61 @@
   auto StyleTd = getStyle("file", "x.td", "llvm", "", &FS);
   ASSERT_TRUE((bool)StyleTd);
   ASSERT_EQ(*StyleTd, getLLVMStyle(FormatStyle::LK_TableGen));
+
+  // Test 9: explicit format file in parent directory.
+  ASSERT_TRUE(
+  FS.addFile("/e/.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: LLVM")));
+  ASSERT_TRUE(
+  FS.addFile("/e/explicit.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
+  ASSERT_TRUE(FS.addFile("/e/sub/sub/sub/test.cpp", 0,
+ llvm::MemoryBuffer::getMemBuffer("int i;")));
+  auto Style8 = getStyle("file:/e/explicit.clang-format",
+ "/e/sub/sub/sub/test.cpp", "LLVM", "", &FS);
+  ASSERT_TRUE((bool)Style8);
+  ASSERT_EQ(*Style8, getGoogleStyle());
+
+  // Test 10: relative pah to a format file
+  ASSERT_TRUE(
+  FS.addFile("../../e/explicit.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
+  auto Style9 = getStyle("file:../../e/explicit.clang-format",
+ "/e/sub/sub/sub/test.cpp", "LLVM", "", &FS);
+  ASSERT_TRUE((bool)Style9);
+  ASSERT_EQ(*Style9, getGoogleStyle());
+
+  // Test 11: missing explicit format file
+  auto Style10 = getStyle("file:/e/missing.clang-format",
+  "/e/sub/sub/sub/test.cpp", "LLVM", "", &FS);
+  ASSERT_FALSE((bool)Style10);
+  llvm::consumeError(Style10.takeError());
+
+  // Test 12: format file from the filesystem
+  SmallString<128> FormatFilePath;
+  std::error_code ECF = llvm::sys::fs::createTemporaryFile(
+  "FormatFileTest", "tpl", FormatFilePath);
+  EXPECT_FALSE((bool)ECF);
+  llvm::raw_fd_ostream FormatFileTest(FormatFilePath, ECF);
+  EXPECT_FALSE((bool)ECF);
+  FormatFileTest << "BasedOnStyle: Google\n";
+  FormatFileTest.close();
+
+  SmallString<128> TestFilePath;
+  std::error_code ECT =
+  llvm::sys::fs::createTemporaryFile("CodeFileTest", "cc", TestFilePath);
+  EXPECT_FALSE((bool)ECT);
+  llvm::raw_fd_ostream CodeFileTest(TestFilePath, ECT);
+  CodeFileTest << "int i;\n";
+  CodeFileTest.close();
+
+  std::string format_file_arg = std::string("file:") + FormatFilePath.c_str();
+  auto Style11 = getStyle(format_file_arg, TestFilePath, "LLVM", "", nullptr);
+
+  llvm::sys::fs::remove(FormatFilePath.c_str());
+  llvm::sys::fs::remove(TestFilePath.c_str());
+  ASSERT_TRUE((bool)Style11);
+  ASSERT_EQ(*Style11, getGoogleStyle());
 }
 
 TEST_F(ReplacementTest, FormatCodeAfterReplacements) {
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -2640,6 +2640,8 @@
 ".clang-format file located in one of the parent\n"
 "directories of the source file (or current\n"
 "directory for stdin).\n"
+"Use -style=file: to explicitly specify"
+"the configuration file.\n"
 "Use -style=\"{key: value, ...}\" to set specific\n"
 "parameters, e.g.:\n"
 "  -style=\"{BasedOnStyle: llvm, IndentWidth: 8}\"";
@@ -2685,6 +2687,29 @@
   return GuessedLanguage;
 }
 
+/// Attempts to load a format file
+llvm::Expected LoadConfigFile(StringRef ConfigFile,
+   llvm::vfs::FileSystem *FS,
+   bool *IsSuitable) {
+  FormatStyle Style = getLLVMStyle();
+  *IsSuitable = true;
+
+  llvm::ErrorOr> Text =
+  FS->getBufferForFile(ConfigFile.str());
+  if (std::error_code EC = Text.getError())
+return make_string_error(EC.message());
+  std::error_code ParserErrorCode =
+  parseConfiguration(Text.get()->getBuffer(), &Style);
+  if (ParserErrorCode == ParseError::Unsuitable) {
+*IsSuitable = false;
+return Style;
+  } else if (ParserErrorCode != ParseError::Success) {
+return make_string_error("Error reading " + ConfigFile + ": " +
+ ParserErrorCode.message());
+  }
+  return Style;
+}
+
 const char *DefaultFormatStyle = "file";
 
 const char *DefaultFallbackStyle = "LLVM";
@@ -2709,6 +2734,21 @@
 return Style;
   }
 
+  llvm::SmallVector FilesToLookFor;
+  // User provided clang-format file using -style=file:/p

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[Mitchell via Phabricator via cfe-commits]

mitchell-stellar added a comment.

It's not more approval that is needed, it's just that someone with commit 
access (assuming you do not) needs to find the time to commit this. For what 
it's worth, I'm getting a patch rejection for the 4th block in 
lib/Format/Format.cpp. It seems the contents of `LoadConfigFile` need to be 
updated to reflect the most recent changes, so please rebase against master 
when you can.


Repository:
  rC Clang

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

https://reviews.llvm.org/D72326



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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[Thibault North via Phabricator via cfe-commits]

tnorth added a comment.

Thanks @MyDeveloperDay . I guess more approvals are needed at this point?


Repository:
  rC Clang

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

https://reviews.llvm.org/D72326



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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay accepted this revision.
MyDeveloperDay added a comment.

LGTM


Repository:
  rC Clang

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

https://reviews.llvm.org/D72326



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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[Thibault North via Phabricator via cfe-commits]

tnorth added a comment.

Ping.


Repository:
  rC Clang

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

https://reviews.llvm.org/D72326



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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[Thibault North via Phabricator via cfe-commits]

tnorth marked 2 inline comments as done.
tnorth added inline comments.



Comment at: include/clang/Format/Format.h:2341
 /// directory if ``FileName`` is empty.
+/// * "file=" to explicitly specify the configuration file to use.
 ///

lebedev.ri wrote:
> So is it `:` or `=`?
`:` is correct, fixed.




Repository:
  rC Clang

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

https://reviews.llvm.org/D72326



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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[Thibault North via Phabricator via cfe-commits]

tnorth updated this revision to Diff 245592.
tnorth added a comment.

Fix comment to file: instead of the incorrect file=


Repository:
  rC Clang

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

https://reviews.llvm.org/D72326

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

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -14350,6 +14350,61 @@
   auto StyleTd = getStyle("file", "x.td", "llvm", "", &FS);
   ASSERT_TRUE((bool)StyleTd);
   ASSERT_EQ(*StyleTd, getLLVMStyle(FormatStyle::LK_TableGen));
+
+  // Test 9: explicit format file in parent directory.
+  ASSERT_TRUE(
+  FS.addFile("/e/.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: LLVM")));
+  ASSERT_TRUE(
+  FS.addFile("/e/explicit.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
+  ASSERT_TRUE(FS.addFile("/e/sub/sub/sub/test.cpp", 0,
+ llvm::MemoryBuffer::getMemBuffer("int i;")));
+  auto Style8 = getStyle("file:/e/explicit.clang-format",
+ "/e/sub/sub/sub/test.cpp", "LLVM", "", &FS);
+  ASSERT_TRUE((bool)Style8);
+  ASSERT_EQ(*Style8, getGoogleStyle());
+
+  // Test 10: relative pah to a format file
+  ASSERT_TRUE(
+  FS.addFile("../../e/explicit.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
+  auto Style9 = getStyle("file:../../e/explicit.clang-format",
+ "/e/sub/sub/sub/test.cpp", "LLVM", "", &FS);
+  ASSERT_TRUE((bool)Style9);
+  ASSERT_EQ(*Style9, getGoogleStyle());
+
+  // Test 11: missing explicit format file
+  auto Style10 = getStyle("file:/e/missing.clang-format",
+  "/e/sub/sub/sub/test.cpp", "LLVM", "", &FS);
+  ASSERT_FALSE((bool)Style10);
+  llvm::consumeError(Style10.takeError());
+
+  // Test 12: format file from the filesystem
+  SmallString<128> FormatFilePath;
+  std::error_code ECF = llvm::sys::fs::createTemporaryFile(
+  "FormatFileTest", "tpl", FormatFilePath);
+  EXPECT_FALSE((bool)ECF);
+  llvm::raw_fd_ostream FormatFileTest(FormatFilePath, ECF);
+  EXPECT_FALSE((bool)ECF);
+  FormatFileTest << "BasedOnStyle: Google\n";
+  FormatFileTest.close();
+
+  SmallString<128> TestFilePath;
+  std::error_code ECT =
+  llvm::sys::fs::createTemporaryFile("CodeFileTest", "cc", TestFilePath);
+  EXPECT_FALSE((bool)ECT);
+  llvm::raw_fd_ostream CodeFileTest(TestFilePath, ECT);
+  CodeFileTest << "int i;\n";
+  CodeFileTest.close();
+
+  std::string format_file_arg = std::string("file:") + FormatFilePath.c_str();
+  auto Style11 = getStyle(format_file_arg, TestFilePath, "LLVM", "", nullptr);
+
+  llvm::sys::fs::remove(FormatFilePath.c_str());
+  llvm::sys::fs::remove(TestFilePath.c_str());
+  ASSERT_TRUE((bool)Style11);
+  ASSERT_EQ(*Style11, getGoogleStyle());
 }
 
 TEST_F(ReplacementTest, FormatCodeAfterReplacements) {
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -2519,6 +2519,8 @@
 ".clang-format file located in one of the parent\n"
 "directories of the source file (or current\n"
 "directory for stdin).\n"
+"Use -style=file: to explicitly specify"
+"the configuration file.\n"
 "Use -style=\"{key: value, ...}\" to set specific\n"
 "parameters, e.g.:\n"
 "  -style=\"{BasedOnStyle: llvm, IndentWidth: 8}\"";
@@ -2568,6 +2570,26 @@
 
 const char *DefaultFallbackStyle = "LLVM";
 
+/// Attempts to load a format file
+llvm::Expected LoadConfigFile(StringRef ConfigFile,
+   llvm::vfs::FileSystem *FS) {
+  llvm::ErrorOr> Text =
+  FS->getBufferForFile(ConfigFile.str());
+  std::error_code ReadFileError = Text.getError();
+  if (ReadFileError) {
+return make_string_error("Error reading config file " +
+ ReadFileError.message());
+  }
+  FormatStyle Style = getLLVMStyle();
+  std::error_code ParseFileError =
+  parseConfiguration(Text.get()->getBuffer(), &Style);
+  if (ParseFileError != ParseError::Success) {
+return make_string_error("Error parsing config file " +
+ ParseFileError.message());
+  }
+  return Style;
+}
+
 llvm::Expected getStyle(StringRef StyleName, StringRef FileName,
  StringRef FallbackStyleName,
  StringRef Code,
@@ -2588,6 +2610,12 @@
 return Style;
   }
 
+  // User provided clang-format file using -style=file:/path/to/format/file
+  // Check for explicit config filename
+  if (StyleName.startswith_lower("file:")) {
+return LoadConfigFile(StyleName.substr(5), FS);
+  }
+
   if (!

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: include/clang/Format/Format.h:2341
 /// directory if ``FileName`` is empty.
+/// * "file=" to explicitly specify the configuration file to use.
 ///

So is it `:` or `=`?


Repository:
  rC Clang

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

https://reviews.llvm.org/D72326



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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[Thibault North via Phabricator via cfe-commits]

tnorth added a comment.

Ping. I guess we need more approval to go forward?


Repository:
  rC Clang

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

https://reviews.llvm.org/D72326



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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[MyDeveloperDay via Phabricator via cfe-commits]

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

In the absence of any other comments, This LGTM, this may help to resolve 
D68569: [clang-format] Also look for .{ext}.clang-format file 
, I think I prefer this solution.


Repository:
  rC Clang

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

https://reviews.llvm.org/D72326



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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[Thibault North via Phabricator via cfe-commits]

tnorth added a comment.

Ping.


Repository:
  rC Clang

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

https://reviews.llvm.org/D72326



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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[Thibault North via Phabricator via cfe-commits]

tnorth updated this revision to Diff 241155.
tnorth added a comment.

- Add release notes
- Update ClangFormat.rst and ClangFormatStyleOption.rst


Repository:
  rC Clang

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

https://reviews.llvm.org/D72326

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

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -14350,6 +14350,61 @@
   auto StyleTd = getStyle("file", "x.td", "llvm", "", &FS);
   ASSERT_TRUE((bool)StyleTd);
   ASSERT_EQ(*StyleTd, getLLVMStyle(FormatStyle::LK_TableGen));
+
+  // Test 9: explicit format file in parent directory.
+  ASSERT_TRUE(
+  FS.addFile("/e/.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: LLVM")));
+  ASSERT_TRUE(
+  FS.addFile("/e/explicit.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
+  ASSERT_TRUE(FS.addFile("/e/sub/sub/sub/test.cpp", 0,
+ llvm::MemoryBuffer::getMemBuffer("int i;")));
+  auto Style8 = getStyle("file:/e/explicit.clang-format",
+ "/e/sub/sub/sub/test.cpp", "LLVM", "", &FS);
+  ASSERT_TRUE((bool)Style8);
+  ASSERT_EQ(*Style8, getGoogleStyle());
+
+  // Test 10: relative pah to a format file
+  ASSERT_TRUE(
+  FS.addFile("../../e/explicit.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
+  auto Style9 = getStyle("file:../../e/explicit.clang-format",
+ "/e/sub/sub/sub/test.cpp", "LLVM", "", &FS);
+  ASSERT_TRUE((bool)Style9);
+  ASSERT_EQ(*Style9, getGoogleStyle());
+
+  // Test 11: missing explicit format file
+  auto Style10 = getStyle("file:/e/missing.clang-format",
+  "/e/sub/sub/sub/test.cpp", "LLVM", "", &FS);
+  ASSERT_FALSE((bool)Style10);
+  llvm::consumeError(Style10.takeError());
+
+  // Test 12: format file from the filesystem
+  SmallString<128> FormatFilePath;
+  std::error_code ECF = llvm::sys::fs::createTemporaryFile(
+  "FormatFileTest", "tpl", FormatFilePath);
+  EXPECT_FALSE((bool)ECF);
+  llvm::raw_fd_ostream FormatFileTest(FormatFilePath, ECF);
+  EXPECT_FALSE((bool)ECF);
+  FormatFileTest << "BasedOnStyle: Google\n";
+  FormatFileTest.close();
+
+  SmallString<128> TestFilePath;
+  std::error_code ECT =
+  llvm::sys::fs::createTemporaryFile("CodeFileTest", "cc", TestFilePath);
+  EXPECT_FALSE((bool)ECT);
+  llvm::raw_fd_ostream CodeFileTest(TestFilePath, ECT);
+  CodeFileTest << "int i;\n";
+  CodeFileTest.close();
+
+  std::string format_file_arg = std::string("file:") + FormatFilePath.c_str();
+  auto Style11 = getStyle(format_file_arg, TestFilePath, "LLVM", "", nullptr);
+
+  llvm::sys::fs::remove(FormatFilePath.c_str());
+  llvm::sys::fs::remove(TestFilePath.c_str());
+  ASSERT_TRUE((bool)Style11);
+  ASSERT_EQ(*Style11, getGoogleStyle());
 }
 
 TEST_F(ReplacementTest, FormatCodeAfterReplacements) {
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -2519,6 +2519,8 @@
 ".clang-format file located in one of the parent\n"
 "directories of the source file (or current\n"
 "directory for stdin).\n"
+"Use -style=file: to explicitly specify"
+"the configuration file.\n"
 "Use -style=\"{key: value, ...}\" to set specific\n"
 "parameters, e.g.:\n"
 "  -style=\"{BasedOnStyle: llvm, IndentWidth: 8}\"";
@@ -2568,6 +2570,26 @@
 
 const char *DefaultFallbackStyle = "LLVM";
 
+/// Attempts to load a format file
+llvm::Expected LoadConfigFile(StringRef ConfigFile,
+   llvm::vfs::FileSystem *FS) {
+  llvm::ErrorOr> Text =
+  FS->getBufferForFile(ConfigFile.str());
+  std::error_code ReadFileError = Text.getError();
+  if (ReadFileError) {
+return make_string_error("Error reading config file " +
+ ReadFileError.message());
+  }
+  FormatStyle Style = getLLVMStyle();
+  std::error_code ParseFileError =
+  parseConfiguration(Text.get()->getBuffer(), &Style);
+  if (ParseFileError != ParseError::Success) {
+return make_string_error("Error parsing config file " +
+ ParseFileError.message());
+  }
+  return Style;
+}
+
 llvm::Expected getStyle(StringRef StyleName, StringRef FileName,
  StringRef FallbackStyleName,
  StringRef Code,
@@ -2588,6 +2610,12 @@
 return Style;
   }
 
+  // User provided clang-format file using -style=file:/path/to/format/file
+  // Check for explicit config filename
+  if (StyleName.startswith_lower("file:")) {
+return LoadConfigFile(StyleName.substr(

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[Thibault North via Phabricator via cfe-commits]

tnorth added a comment.

In D72326#1845342 , @MyDeveloperDay 
wrote:

> Nit: please add a release note and regenerate the ClangFormatStyleOptions.rst 
> (if there are any changes because you modified Format.h).


Hmm, I tried to run docs/tools/dump_format_style.py, but it fails at two 
locations (once because a comment in Format.h has two slashes instead of 3, one 
because of an empty line). After fixing those, it seems that the generated file 
contains less information than the commited one. This file was probably updated 
manually as well. Therefore I also did in the updated diff... one should 
probably fix this separately.
Release note + ClangFormat.rst files update in the diff as well.


Repository:
  rC Clang

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

https://reviews.llvm.org/D72326



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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

Nit: please add a release note and regenerate the ClangFormatStyleOptions.rst 
(if there are any changes because you modified Format.h).


Repository:
  rC Clang

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

https://reviews.llvm.org/D72326



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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[Thibault North via Phabricator via cfe-commits]

tnorth marked 7 inline comments as done.
tnorth added a comment.

> This makes sense for command-line args, but if I understand correctly this 
> patch will also allow BasedOnStyle: file:some/path. Is that the case?

No, it should not, and I also think it's better not to.

I think that all points are addressed now. Looking forward to have your 
feedback.


Repository:
  rC Clang

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

https://reviews.llvm.org/D72326



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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[Thibault North via Phabricator via cfe-commits]

tnorth updated this revision to Diff 240888.
tnorth added a comment.

- Add a unit-test loading a format and test file from temporary files.


Repository:
  rC Clang

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

https://reviews.llvm.org/D72326

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -14350,6 +14350,59 @@
   auto StyleTd = getStyle("file", "x.td", "llvm", "", &FS);
   ASSERT_TRUE((bool)StyleTd);
   ASSERT_EQ(*StyleTd, getLLVMStyle(FormatStyle::LK_TableGen));
+
+  // Test 9: explicit format file in parent directory.
+  ASSERT_TRUE(
+  FS.addFile("/e/.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: LLVM")));
+  ASSERT_TRUE(
+  FS.addFile("/e/explicit.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
+  ASSERT_TRUE(FS.addFile("/e/sub/sub/sub/test.cpp", 0,
+ llvm::MemoryBuffer::getMemBuffer("int i;")));
+  auto Style8 = getStyle("file:/e/explicit.clang-format",
+ "/e/sub/sub/sub/test.cpp", "LLVM", "", &FS);
+  ASSERT_TRUE((bool)Style8);
+  ASSERT_EQ(*Style8, getGoogleStyle());
+
+  // Test 10: relative pah to a format file
+  ASSERT_TRUE(
+  FS.addFile("../../e/explicit.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
+  auto Style9 = getStyle("file:../../e/explicit.clang-format",
+ "/e/sub/sub/sub/test.cpp", "LLVM", "", &FS);
+  ASSERT_TRUE((bool)Style9);
+  ASSERT_EQ(*Style9, getGoogleStyle());
+
+  // Test 11: missing explicit format file
+  auto Style10 = getStyle("file:/e/missing.clang-format",
+  "/e/sub/sub/sub/test.cpp", "LLVM", "", &FS);
+  ASSERT_FALSE((bool)Style10);
+  llvm::consumeError(Style10.takeError());
+
+  // Test 12: format file from the filesystem
+  SmallString<128> FormatFilePath;
+  std::error_code ECF = llvm::sys::fs::createTemporaryFile("FormatFileTest", "tpl", FormatFilePath);
+  EXPECT_FALSE((bool)ECF);
+  llvm::raw_fd_ostream  FormatFileTest(FormatFilePath, ECF);
+  EXPECT_FALSE((bool)ECF);
+  FormatFileTest << "BasedOnStyle: Google\n";
+  FormatFileTest.close();
+
+  SmallString<128> TestFilePath;
+  std::error_code ECT = llvm::sys::fs::createTemporaryFile("CodeFileTest", "cc", TestFilePath);
+  EXPECT_FALSE((bool)ECT);
+  llvm::raw_fd_ostream  CodeFileTest(TestFilePath, ECT);
+  CodeFileTest << "int i;\n";
+  CodeFileTest.close();
+
+  std::string format_file_arg = std::string("file:") + FormatFilePath.c_str();
+  auto Style11 = getStyle(format_file_arg, TestFilePath, "LLVM", "", nullptr);
+  
+  llvm::sys::fs::remove(FormatFilePath.c_str());
+  llvm::sys::fs::remove(TestFilePath.c_str());
+  ASSERT_TRUE((bool)Style11);
+  ASSERT_EQ(*Style11, getGoogleStyle());
 }
 
 TEST_F(ReplacementTest, FormatCodeAfterReplacements) {
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -2519,6 +2519,8 @@
 ".clang-format file located in one of the parent\n"
 "directories of the source file (or current\n"
 "directory for stdin).\n"
+"Use -style=file: to explicitly specify"
+"the configuration file.\n"
 "Use -style=\"{key: value, ...}\" to set specific\n"
 "parameters, e.g.:\n"
 "  -style=\"{BasedOnStyle: llvm, IndentWidth: 8}\"";
@@ -2568,6 +2570,22 @@
 
 const char *DefaultFallbackStyle = "LLVM";
 
+/// Attempts to load a format file
+llvm::Expected LoadConfigFile(StringRef ConfigFile, llvm::vfs::FileSystem *FS) {
+  llvm::ErrorOr> Text =
+  FS->getBufferForFile(ConfigFile.str());
+  std::error_code ReadFileError = Text.getError();
+  if (ReadFileError) {
+return make_string_error("Error reading config file " + ReadFileError.message());
+  }
+  FormatStyle Style = getLLVMStyle();
+  std::error_code ParseFileError = parseConfiguration(Text.get()->getBuffer(), &Style);
+  if (ParseFileError != ParseError::Success) {
+return make_string_error("Error parsing config file " + ParseFileError.message());
+  }
+  return Style;
+}
+
 llvm::Expected getStyle(StringRef StyleName, StringRef FileName,
  StringRef FallbackStyleName,
  StringRef Code,
@@ -2588,6 +2606,12 @@
 return Style;
   }
 
+  // User provided clang-format file using -style=file:/path/to/format/file
+  // Check for explicit config filename
+  if (StyleName.startswith_lower("file:")) {
+return LoadConfigFile(StyleName.substr(5), FS);
+  }
+
   if (!StyleName.equals_lower("file")) {
 if (!getPredefinedStyle(StyleName, Style.Language, &Style))
   return make_string_error("Invalid value for -style");
@@ -2628,24 +2652,7 @@
  

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[Thibault North via Phabricator via cfe-commits]

tnorth updated this revision to Diff 238513.
tnorth added a comment.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Thanks for your time and feedback.

- Update diff with context,
- remove public declaration of LoadConfigFile,
- change prototype of LoadConfigFile and simplify it,
- avoid string copy to ConfigFile, use StringRef directly,
- don't stat the file before attempting to read it.

Regarding the tests, I am unsure how to do this currenlty (I'd need to read 
some docs/other examples).
I didn't have in mind the use case of BasedOnStyle: file:some/path.


Repository:
  rC Clang

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

https://reviews.llvm.org/D72326

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -14350,6 +14350,35 @@
   auto StyleTd = getStyle("file", "x.td", "llvm", "", &FS);
   ASSERT_TRUE((bool)StyleTd);
   ASSERT_EQ(*StyleTd, getLLVMStyle(FormatStyle::LK_TableGen));
+
+  // Test 9: explicit format file in parent directory.
+  ASSERT_TRUE(
+  FS.addFile("/e/.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: LLVM")));
+  ASSERT_TRUE(
+  FS.addFile("/e/explicit.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
+  ASSERT_TRUE(FS.addFile("/e/sub/sub/sub/test.cpp", 0,
+ llvm::MemoryBuffer::getMemBuffer("int i;")));
+  auto Style8 = getStyle("file:/e/explicit.clang-format",
+ "/e/sub/sub/sub/test.cpp", "LLVM", "", &FS);
+  ASSERT_TRUE((bool)Style8);
+  ASSERT_EQ(*Style8, getGoogleStyle());
+
+  // Test 10: relative pah to a format file
+  ASSERT_TRUE(
+  FS.addFile("../../e/explicit.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
+  auto Style9 = getStyle("file:../../e/explicit.clang-format",
+ "/e/sub/sub/sub/test.cpp", "LLVM", "", &FS);
+  ASSERT_TRUE((bool)Style9);
+  ASSERT_EQ(*Style9, getGoogleStyle());
+
+  // Test 11: missing explicit format file
+  auto Style10 = getStyle("file:/e/missing.clang-format",
+  "/e/sub/sub/sub/test.cpp", "LLVM", "", &FS);
+  ASSERT_FALSE((bool)Style10);
+  llvm::consumeError(Style10.takeError());
 }
 
 TEST_F(ReplacementTest, FormatCodeAfterReplacements) {
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -2519,6 +2519,8 @@
 ".clang-format file located in one of the parent\n"
 "directories of the source file (or current\n"
 "directory for stdin).\n"
+"Use -style=file: to explicitly specify"
+"the configuration file.\n"
 "Use -style=\"{key: value, ...}\" to set specific\n"
 "parameters, e.g.:\n"
 "  -style=\"{BasedOnStyle: llvm, IndentWidth: 8}\"";
@@ -2568,6 +2570,22 @@
 
 const char *DefaultFallbackStyle = "LLVM";
 
+/// Attempts to load a format file
+llvm::Expected LoadConfigFile(StringRef ConfigFile, llvm::vfs::FileSystem *FS) {
+  llvm::ErrorOr> Text =
+  FS->getBufferForFile(ConfigFile.str());
+  std::error_code ReadFileError = Text.getError();
+  if (ReadFileError) {
+return make_string_error("Error reading config file " + ReadFileError.message());
+  }
+  FormatStyle Style = getLLVMStyle();
+  std::error_code ParseFileError = parseConfiguration(Text.get()->getBuffer(), &Style);
+  if (ParseFileError != ParseError::Success) {
+return make_string_error("Error parsing config file " + ParseFileError.message());
+  }
+  return Style;
+}
+
 llvm::Expected getStyle(StringRef StyleName, StringRef FileName,
  StringRef FallbackStyleName,
  StringRef Code,
@@ -2588,6 +2606,12 @@
 return Style;
   }
 
+  // User provided clang-format file using -style=file:/path/to/format/file
+  // Check for explicit config filename
+  if (StyleName.startswith_lower("file:")) {
+return LoadConfigFile(StyleName.substr(5), FS);
+  }
+
   if (!StyleName.equals_lower("file")) {
 if (!getPredefinedStyle(StyleName, Style.Language, &Style))
   return make_string_error("Invalid value for -style");
@@ -2628,24 +2652,7 @@
 }
 
 if (FoundConfigFile) {
-  llvm::ErrorOr> Text =
-  FS->getBufferForFile(ConfigFile.str());
-  if (std::error_code EC = Text.getError())
-return make_string_error(EC.message());
-  if (std::error_code ec =
-  parseConfiguration(Text.get()->getBuffer(), &Style)) {
-if (ec == ParseError::Unsuitable) {
-  if (!UnsuitableConfigFiles.empty())
-UnsuitableConfigFiles.append(", ");
-  UnsuitableConfigFiles.append(ConfigFile);
-  continue;
-}
-