[PATCH] D115604: [Support] Expand `` as the base directory in response files.

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added a comment.

The name of the patch could be more precise if you change `response files` to 
`configuration files`.




Comment at: clang/docs/UsersManual.rst:926-927
+
+A potential `` use-case may be search paths in a portable (i.e. not
+installed) SDK directory for embedded development. The user may only need to
+specify a root configuration file to establish every aspect of the SDK with the

It does not describe the use case clearly. Such macro is useful in the cases 
when configuration file is kept together with SDK content so that location of 
various SDK component can be expressed using the file location. Something about 
that could be provided here.



Comment at: llvm/include/llvm/Support/CommandLine.h:2117
+ llvm::vfs::FileSystem ,
+ StringRef ExpandBasePathToken = {});
 

It seems that this parameter can be boolean. It only instructs to call 
`ExpandBasePaths`, which knows what to expand.



Comment at: llvm/include/llvm/Support/CommandLine.h:2126
+llvm::Optional CurrentDir = llvm::None,
+StringRef ExpandBasePathToken = {});
 

The same about boolean parameter.



Comment at: llvm/lib/Support/CommandLine.cpp:1082
+// Substitute token with the file's base path.
+static void ExpandBasePaths(StringRef BasePath, StringRef Token,
+StringSaver , const char *) {

It seems that the parameter `Token` is not needed, as it is always ``.



Comment at: llvm/lib/Support/CommandLine.cpp:1118
+  llvm::vfs::FileSystem ,
+  StringRef ExpandBasePathToken) {
   assert(sys::path::is_absolute(FName));

It also could be boolean.



Comment at: llvm/lib/Support/CommandLine.cpp:1184
+ llvm::vfs::FileSystem ,
+ StringRef ExpandBasePathToken) {
   bool AllExpanded = true;

It could be boolean.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115604

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

[PATCH] D115604: [Support] Expand `` as the base directory in response files.

[Jack Andersen via Phabricator via cfe-commits]

jackoalan updated this revision to Diff 396410.
jackoalan added a comment.

Rebase, use the slightly more intuitive `` token to expand base paths.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115604

Files:
  clang/docs/ReleaseNotes.rst
  clang/docs/UsersManual.rst
  llvm/include/llvm/Support/CommandLine.h
  llvm/lib/Support/CommandLine.cpp
  llvm/unittests/Support/CommandLineTest.cpp

Index: llvm/unittests/Support/CommandLineTest.cpp
===
--- llvm/unittests/Support/CommandLineTest.cpp
+++ llvm/unittests/Support/CommandLineTest.cpp
@@ -1038,25 +1038,34 @@
   llvm::SmallVector Argv;
 
   TempDir TestDir("unittest", /*Unique*/ true);
+  TempDir TestSubDir(TestDir.path("subdir"), /*Unique*/ false);
 
-  llvm::SmallString<128> TestCfg;
-  llvm::sys::path::append(TestCfg, TestDir.path(), "foo");
-
+  llvm::SmallString<128> TestCfg = TestDir.path("foo");
   TempFile ConfigFile(TestCfg, "",
   "# Comment\n"
   "-option_1\n"
+  "-option_2=/dir1\n"
   "@subconfig\n"
-  "-option_3=abcd\n"
-  "-option_4=\\\n"
+  "-option_7=abcd\n"
+  "-option_8=\\\n"
   "cdef\n");
 
-  llvm::SmallString<128> TestCfg2;
-  llvm::sys::path::append(TestCfg2, TestDir.path(), "subconfig");
+  llvm::SmallString<128> TestCfg2 = TestDir.path("subconfig");
   TempFile ConfigFile2(TestCfg2, "",
-   "-option_2\n"
+   "-option_3\n"
+   "-option_4=/dir2\n"
+   "@subdir/subfoo\n"
"\n"
"   # comment\n");
 
+  llvm::SmallString<128> TestCfg3 = TestSubDir.path("subfoo");
+  TempFile ConfigFile3(TestCfg3, "",
+   "-option_5=/dir3\n"
+   "@/subfoo2\n");
+
+  llvm::SmallString<128> TestCfg4 = TestSubDir.path("subfoo2");
+  TempFile ConfigFile4(TestCfg4, "", "-option_6=qwerty\n");
+
   // Make sure the current directory is not the directory where config files
   // resides. In this case the code that expands response files will not find
   // 'subconfig' unless it resolves nested inclusions relative to the including
@@ -1071,11 +1080,18 @@
   bool Result = llvm::cl::readConfigFile(ConfigFile.path(), Saver, Argv);
 
   EXPECT_TRUE(Result);
-  EXPECT_EQ(Argv.size(), 4U);
+  EXPECT_EQ(Argv.size(), 8U);
   EXPECT_STREQ(Argv[0], "-option_1");
-  EXPECT_STREQ(Argv[1], "-option_2");
-  EXPECT_STREQ(Argv[2], "-option_3=abcd");
-  EXPECT_STREQ(Argv[3], "-option_4=cdef");
+  EXPECT_STREQ(Argv[1],
+   ("-option_2=" + TestDir.path() + "/dir1").str().c_str());
+  EXPECT_STREQ(Argv[2], "-option_3");
+  EXPECT_STREQ(Argv[3],
+   ("-option_4=" + TestDir.path() + "/dir2").str().c_str());
+  EXPECT_STREQ(Argv[4],
+   ("-option_5=" + TestSubDir.path() + "/dir3").str().c_str());
+  EXPECT_STREQ(Argv[5], "-option_6=qwerty");
+  EXPECT_STREQ(Argv[6], "-option_7=abcd");
+  EXPECT_STREQ(Argv[7], "-option_8=cdef");
 }
 
 TEST(CommandLineTest, PositionalEatArgsError) {
Index: llvm/lib/Support/CommandLine.cpp
===
--- llvm/lib/Support/CommandLine.cpp
+++ llvm/lib/Support/CommandLine.cpp
@@ -1078,11 +1078,44 @@
   return (S.size() >= 3 && S[0] == '\xef' && S[1] == '\xbb' && S[2] == '\xbf');
 }
 
+// Substitute token with the file's base path.
+static void ExpandBasePaths(StringRef BasePath, StringRef Token,
+StringSaver , const char *) {
+  assert(sys::path::is_absolute(BasePath));
+  const StringRef ArgString(Arg);
+
+  SmallString<128> ResponseFile;
+  StringRef::size_type StartPos = 0;
+  for (StringRef::size_type TokenPos = ArgString.find(Token);
+   TokenPos != StringRef::npos;
+   TokenPos = ArgString.find(Token, StartPos)) {
+// Token may appear more than once per arg (e.g. comma-separated linker
+// args). Support by using path-append on any subsequent appearances.
+const StringRef LHS = ArgString.substr(StartPos, TokenPos - StartPos);
+if (ResponseFile.empty())
+  ResponseFile = LHS;
+else
+  llvm::sys::path::append(ResponseFile, LHS);
+ResponseFile.append(BasePath);
+StartPos = TokenPos + Token.size();
+  }
+
+  if (!ResponseFile.empty()) {
+// Path-append the remaining arg substring if at least one token appeared.
+const StringRef Remaining = ArgString.substr(StartPos);
+if (!Remaining.empty())
+  llvm::sys::path::append(ResponseFile, Remaining);
+Arg = Saver.save(ResponseFile.str()).data();
+  }
+}
+
 // FName must be an absolute path.
-static llvm::Error ExpandResponseFile(
-StringRef FName, StringSaver , TokenizerCallback Tokenizer,
-SmallVectorImpl , bool MarkEOLs, bool 

[PATCH] D115604: [Support] Expand `<@>` as the base directory in response files.

[Jack Andersen via Phabricator via cfe-commits]

jackoalan updated this revision to Diff 396055.
jackoalan added a comment.

Make expansion token a parameter of `ExpandResponseFiles` and limit use to 
`readConfigFile`. Elaborate manual entry further and add entry to release notes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115604

Files:
  clang/docs/ReleaseNotes.rst
  clang/docs/UsersManual.rst
  llvm/include/llvm/Support/CommandLine.h
  llvm/lib/Support/CommandLine.cpp
  llvm/unittests/Support/CommandLineTest.cpp

Index: llvm/unittests/Support/CommandLineTest.cpp
===
--- llvm/unittests/Support/CommandLineTest.cpp
+++ llvm/unittests/Support/CommandLineTest.cpp
@@ -1038,25 +1038,34 @@
   llvm::SmallVector Argv;
 
   TempDir TestDir("unittest", /*Unique*/ true);
+  TempDir TestSubDir(TestDir.path("subdir"), /*Unique*/ false);
 
-  llvm::SmallString<128> TestCfg;
-  llvm::sys::path::append(TestCfg, TestDir.path(), "foo");
-
+  llvm::SmallString<128> TestCfg = TestDir.path("foo");
   TempFile ConfigFile(TestCfg, "",
   "# Comment\n"
   "-option_1\n"
+  "-option_2=/dir1\n"
   "@subconfig\n"
-  "-option_3=abcd\n"
-  "-option_4=\\\n"
+  "-option_7=abcd\n"
+  "-option_8=\\\n"
   "cdef\n");
 
-  llvm::SmallString<128> TestCfg2;
-  llvm::sys::path::append(TestCfg2, TestDir.path(), "subconfig");
+  llvm::SmallString<128> TestCfg2 = TestDir.path("subconfig");
   TempFile ConfigFile2(TestCfg2, "",
-   "-option_2\n"
+   "-option_3\n"
+   "-option_4=/dir2\n"
+   "@subdir/subfoo\n"
"\n"
"   # comment\n");
 
+  llvm::SmallString<128> TestCfg3 = TestSubDir.path("subfoo");
+  TempFile ConfigFile3(TestCfg3, "",
+   "-option_5=/dir3\n"
+   "@/subfoo2\n");
+
+  llvm::SmallString<128> TestCfg4 = TestSubDir.path("subfoo2");
+  TempFile ConfigFile4(TestCfg4, "", "-option_6=qwerty\n");
+
   // Make sure the current directory is not the directory where config files
   // resides. In this case the code that expands response files will not find
   // 'subconfig' unless it resolves nested inclusions relative to the including
@@ -1071,11 +1080,18 @@
   bool Result = llvm::cl::readConfigFile(ConfigFile.path(), Saver, Argv);
 
   EXPECT_TRUE(Result);
-  EXPECT_EQ(Argv.size(), 4U);
+  EXPECT_EQ(Argv.size(), 8U);
   EXPECT_STREQ(Argv[0], "-option_1");
-  EXPECT_STREQ(Argv[1], "-option_2");
-  EXPECT_STREQ(Argv[2], "-option_3=abcd");
-  EXPECT_STREQ(Argv[3], "-option_4=cdef");
+  EXPECT_STREQ(Argv[1],
+   ("-option_2=" + TestDir.path() + "/dir1").str().c_str());
+  EXPECT_STREQ(Argv[2], "-option_3");
+  EXPECT_STREQ(Argv[3],
+   ("-option_4=" + TestDir.path() + "/dir2").str().c_str());
+  EXPECT_STREQ(Argv[4],
+   ("-option_5=" + TestSubDir.path() + "/dir3").str().c_str());
+  EXPECT_STREQ(Argv[5], "-option_6=qwerty");
+  EXPECT_STREQ(Argv[6], "-option_7=abcd");
+  EXPECT_STREQ(Argv[7], "-option_8=cdef");
 }
 
 TEST(CommandLineTest, PositionalEatArgsError) {
Index: llvm/lib/Support/CommandLine.cpp
===
--- llvm/lib/Support/CommandLine.cpp
+++ llvm/lib/Support/CommandLine.cpp
@@ -1078,11 +1078,44 @@
   return (S.size() >= 3 && S[0] == '\xef' && S[1] == '\xbb' && S[2] == '\xbf');
 }
 
+// Substitute token with the file's base path.
+static void ExpandBasePaths(StringRef BasePath, StringRef Token,
+StringSaver , const char *) {
+  assert(sys::path::is_absolute(BasePath));
+  const StringRef ArgString(Arg);
+
+  SmallString<128> ResponseFile;
+  StringRef::size_type StartPos = 0;
+  for (StringRef::size_type TokenPos = ArgString.find(Token);
+   TokenPos != StringRef::npos;
+   TokenPos = ArgString.find(Token, StartPos)) {
+// Token may appear more than once per arg (e.g. comma-separated linker
+// args). Support by using path-append on any subsequent appearances.
+const StringRef LHS = ArgString.substr(StartPos, TokenPos - StartPos);
+if (ResponseFile.empty())
+  ResponseFile = LHS;
+else
+  llvm::sys::path::append(ResponseFile, LHS);
+ResponseFile.append(BasePath);
+StartPos = TokenPos + Token.size();
+  }
+
+  if (!ResponseFile.empty()) {
+// Path-append the remaining arg substring if at least one token appeared.
+const StringRef Remaining = ArgString.substr(StartPos);
+if (!Remaining.empty())
+  llvm::sys::path::append(ResponseFile, Remaining);
+Arg = Saver.save(ResponseFile.str()).data();
+  }
+}
+
 // FName must be an absolute path.
-static llvm::Error ExpandResponseFile(
-StringRef FName, StringSaver 

[PATCH] D115604: [Support] Expand `<@>` as the base directory in response files.

[Jack Andersen via Phabricator via cfe-commits]

jackoalan added a comment.

> 1. It it possible to limit the new syntax to config files only? It would 
> avoid concerns of gcc compatibility.

Yes, ultimately this use case only calls for extending config files.

> Is it possible to use more understandable designation, like `` or 
> something similar?

I agree, keyword directives like this would be more flexible in the long term 
and the use of shell redirection characters would theoretically protect any 
general purpose word. For now, I think it is acceptable to consume `` 
as a singular token, but in the future, generic `<(.*)>` tokenization may be 
worth adding.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115604

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

[PATCH] D115604: [Support] Expand `<@>` as the base directory in response files.

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added a comment.

Thank you for the detailed explanation. Now I see how you are going to use this 
facility and think it worth implementation. A couple of questions.

1. It it possible to limit the new syntax to config files only? It would avoid 
concerns of gcc compatibility.

2. The new token is  a weird combination of special symbols, it cannot be 
understood without reading documentation. Is it possible to use more 
understandable designation, like `` or something similar? It would make 
config files more readable and allow future extensions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115604

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

[PATCH] D115604: [Support] Expand `<@>` as the base directory in response files.

[Jack Andersen via Phabricator via cfe-commits]

jackoalan added inline comments.



Comment at: clang/docs/UsersManual.rst:920
 
+To generate paths relative to the configuration file, the `<@>` token may be
+used. This will expand to the absolute path of the directory containing the

sepavloff wrote:
> Response and configuration files are different things, so the documentation 
> must more specific what facility is extended.
> 
> According to this implementation the response file treatment is extended. 
> During development of configuration files couple of attempts were taken to 
> extend the functionality of response files (see for example 
> https://reviews.llvm.org/D36045). They were rejected as breaking 
> compatibility with gcc. It does not mean that such extending is not possible, 
> but it should be discussed in wider community. There must be serious reason 
> for such extension and significant use case.
> 
> As for using this feature for configuration files, it does not look as very 
> helpful. Configuration files are searched for in several places and the first 
> found file is used. In this case specification of various locations relative 
> to the config-file does not look neither safe nor natural. Specific use case 
> also would be helpful to ground the need for such extension.
> Specific use case also would be helpful to ground the need for such extension.

An embedded architecture's community members may wish to deploy their own 
portable (non-installed) platform packages, while minimizing the user burden of 
configuring the build system of their choice to set up all the search paths. 
Ideally, just the `--config` option should be enough to handle everything 
necessary for full use of the package.

Consider an embedded architecture "foo". It deploys a common directory which 
contains a shared configuration file and headers:

```
-foo-common
  -config.cfg
  -include
-foo.h
-stdlib.h
-string.h
-...
```

foo-common/config.cfg:

```
--target=foo
-isystem <@>/include
```

foo is implemented by a family of boards. Each board SDK is contained in a 
directory located next to the common one. Each has their own device libraries 
and some require special feature and machine flags.

Here is the directory layout for a board named "bar":

```
-foo-bar
  -config.cfg
  -include
-bar.h
  -lib
-libbar.a
-libc.a
-libm.a
  -ldscripts
-link.ld
```

foo-bar/config.cfg:

```
@../foo-common/config.cfg
-mcpu=
-isystem <@>/include
-L <@>/lib
-T <@>/ldscripts/link.ld
```

Configuration files are very convenient for setting up compiler options in an 
accessible way (i.e. not having to depend on hard-coded logic in the driver or 
cmake/makefiles/etc to set up the environment). However, the inability to 
express config-relative paths is a major burden for setting up search paths in 
this use case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115604

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

[PATCH] D115604: [Support] Expand `<@>` as the base directory in response files.

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added inline comments.



Comment at: clang/docs/UsersManual.rst:920
 
+To generate paths relative to the configuration file, the `<@>` token may be
+used. This will expand to the absolute path of the directory containing the

Response and configuration files are different things, so the documentation 
must more specific what facility is extended.

According to this implementation the response file treatment is extended. 
During development of configuration files couple of attempts were taken to 
extend the functionality of response files (see for example 
https://reviews.llvm.org/D36045). They were rejected as breaking compatibility 
with gcc. It does not mean that such extending is not possible, but it should 
be discussed in wider community. There must be serious reason for such 
extension and significant use case.

As for using this feature for configuration files, it does not look as very 
helpful. Configuration files are searched for in several places and the first 
found file is used. In this case specification of various locations relative to 
the config-file does not look neither safe nor natural. Specific use case also 
would be helpful to ground the need for such extension.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115604

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

[PATCH] D115604: [Support] Expand `<@>` as the base directory in response files.

[Jack Andersen via Phabricator via cfe-commits]

jackoalan added a comment.

bump, review requested


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115604

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

[PATCH] D115604: [Support] Expand `<@>` as the base directory in response files.

[Jack Andersen via Phabricator via cfe-commits]

jackoalan added a comment.

In D115604#3188392 , @kadircet wrote:

> IIUC, the new behavior being introduced by this patch is not the ability of 
> having a way to refer to other files in a config/response file relative way, 
> but rather extending that ability from only the options that start with `@` 
> to options that contain `@` as a sub-string in any place.

`@` actually behaves as an "include" operation (i.e. the contents of the 
relative file are tokenized into more response file args). It will fail if a 
directory is passed because it is only used by `FileSystem::getBufferForFile`.

The `<@>` operation I am proposing does not perform any file load and simply 
expands to the base path as the function already knows it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115604

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

[PATCH] D115604: [Support] Expand `<@>` as the base directory in response files.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

IIUC, the new behavior being introduced by this patch is not the ability of 
having a way to refer to other files in a config/response file relative way, 
but rather extending that ability from only the options that start with `@` to 
options that contain `@` as a sub-string in any place.

If so, why do we need that exactly? What's wrong with specifying the 
config/response file as:

  --target=sample
  -isystem
   @include
  -L
   @lib
  -T 
  @ldscripts/link.ld

instead of having them on the flag and the value on the same line? are there 
any "real" cases in which you need a `foo=@something` today and that command 
line option doesn't have a form without the `=` in between?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115604

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

[PATCH] D115604: [Support] Expand `<@>` as the base directory in response files.

[Jack Andersen via Phabricator via cfe-commits]

jackoalan updated this revision to Diff 393785.
jackoalan added a comment.

Test coverage for more than one `<@>` per arg.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115604

Files:
  clang/docs/UsersManual.rst
  llvm/lib/Support/CommandLine.cpp
  llvm/unittests/Support/CommandLineTest.cpp

Index: llvm/unittests/Support/CommandLineTest.cpp
===
--- llvm/unittests/Support/CommandLineTest.cpp
+++ llvm/unittests/Support/CommandLineTest.cpp
@@ -784,9 +784,9 @@
 TEST(CommandLineTest, ResponseFiles) {
   vfs::InMemoryFileSystem FS;
 #ifdef _WIN32
-  const char *TestRoot = "C:\\";
+  StringRef TestRoot = "C:\\";
 #else
-  const char *TestRoot = "/";
+  StringRef TestRoot = "/";
 #endif
   FS.setCurrentWorkingDirectory(TestRoot);
 
@@ -796,8 +796,11 @@
  llvm::MemoryBuffer::getMemBuffer("-option_1 -option_2\n"
   "@incdir/resp2\n"
   "-option_3=abcd\n"
-  "@incdir/resp3\n"
-  "-option_4=efjk\n"));
+  "@<@>/incdir/resp3\n"
+  "-option_4=efjk\n"
+  "-option_5=<@>\n"
+  "-option_6=<@>/sub\n"
+  "-option_7=<@>/sub1,<@>/sub2\n"));
 
   // Directory for included file.
   llvm::StringRef IncDir = "incdir";
@@ -807,14 +810,17 @@
   llvm::sys::path::append(IncludedFileName2, IncDir, "resp2");
   FS.addFile(IncludedFileName2, 0,
  MemoryBuffer::getMemBuffer("-option_21 -option_22\n"
-"-option_23=abcd\n"));
+"-option_23=abcd\n"
+"-option_24=<@>\n"
+"-option_25=<@>/sub1,<@>/sub2\n"));
 
   // Create second included response file of second level.
   llvm::SmallString<128> IncludedFileName3;
   llvm::sys::path::append(IncludedFileName3, IncDir, "resp3");
   FS.addFile(IncludedFileName3, 0,
  MemoryBuffer::getMemBuffer("-option_31 -option_32\n"
-"-option_33=abcd\n"));
+"-option_33=abcd\n"
+"-option_34=<@>\n"));
 
   // Prepare 'file' with reference to response file.
   SmallString<128> IncRef;
@@ -828,13 +834,27 @@
   llvm::StringSaver Saver(A);
   ASSERT_TRUE(llvm::cl::ExpandResponseFiles(
   Saver, llvm::cl::TokenizeGNUCommandLine, Argv, false, true,
-  /*CurrentDir=*/StringRef(TestRoot), FS));
-  EXPECT_THAT(Argv, testing::Pointwise(
-StringEquality(),
-{"test/test", "-flag_1", "-option_1", "-option_2",
- "-option_21", "-option_22", "-option_23=abcd",
- "-option_3=abcd", "-option_31", "-option_32",
- "-option_33=abcd", "-option_4=efjk", "-flag_2"}));
+  /*CurrentDir=*/TestRoot, FS));
+
+  std::string ExpectedOption24 = ("-option_24=" + TestRoot + "incdir").str();
+  std::string ExpectedOption25 =
+  ("-option_25=" + TestRoot + "incdir/sub1," + TestRoot + "incdir/sub2")
+  .str();
+  std::string ExpectedOption34 = ("-option_34=" + TestRoot + "incdir").str();
+  std::string ExpectedOption5 = ("-option_5=" + TestRoot).str();
+  std::string ExpectedOption6 = ("-option_6=" + TestRoot + "sub").str();
+  std::string ExpectedOption7 =
+  ("-option_7=" + TestRoot + "sub1," + TestRoot + "sub2").str();
+
+  EXPECT_THAT(
+  Argv, testing::Pointwise(
+StringEquality(),
+{"test/test", "-flag_1", "-option_1", "-option_2", "-option_21",
+ "-option_22", "-option_23=abcd", ExpectedOption24.c_str(),
+ ExpectedOption25.c_str(), "-option_3=abcd", "-option_31",
+ "-option_32", "-option_33=abcd", ExpectedOption34.c_str(),
+ "-option_4=efjk", ExpectedOption5.c_str(),
+ ExpectedOption6.c_str(), ExpectedOption7.c_str(), "-flag_2"}));
 }
 
 TEST(CommandLineTest, RecursiveResponseFiles) {
@@ -1045,15 +1065,17 @@
   TempFile ConfigFile(TestCfg, "",
   "# Comment\n"
   "-option_1\n"
+  "-option_2=<@>/dir1\n"
   "@subconfig\n"
-  "-option_3=abcd\n"
-  "-option_4=\\\n"
+  "-option_5=abcd\n"
+  "-option_6=\\\n"
   "cdef\n");
 
   llvm::SmallString<128> TestCfg2;
   llvm::sys::path::append(TestCfg2, TestDir.path(), "subconfig");
   TempFile ConfigFile2(TestCfg2, "",
-

[PATCH] D115604: [Support] Expand `<@>` as the base directory in response files.

[Jack Andersen via Phabricator via cfe-commits]

jackoalan updated this revision to Diff 393781.
jackoalan added a comment.

Fix ResponseFiles test for Windows.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115604

Files:
  clang/docs/UsersManual.rst
  llvm/lib/Support/CommandLine.cpp
  llvm/unittests/Support/CommandLineTest.cpp

Index: llvm/unittests/Support/CommandLineTest.cpp
===
--- llvm/unittests/Support/CommandLineTest.cpp
+++ llvm/unittests/Support/CommandLineTest.cpp
@@ -784,9 +784,9 @@
 TEST(CommandLineTest, ResponseFiles) {
   vfs::InMemoryFileSystem FS;
 #ifdef _WIN32
-  const char *TestRoot = "C:\\";
+  StringRef TestRoot = "C:\\";
 #else
-  const char *TestRoot = "/";
+  StringRef TestRoot = "/";
 #endif
   FS.setCurrentWorkingDirectory(TestRoot);
 
@@ -796,8 +796,10 @@
  llvm::MemoryBuffer::getMemBuffer("-option_1 -option_2\n"
   "@incdir/resp2\n"
   "-option_3=abcd\n"
-  "@incdir/resp3\n"
-  "-option_4=efjk\n"));
+  "@<@>/incdir/resp3\n"
+  "-option_4=efjk\n"
+  "-option_5=<@>\n"
+  "-option_6=<@>/sub\n"));
 
   // Directory for included file.
   llvm::StringRef IncDir = "incdir";
@@ -807,14 +809,16 @@
   llvm::sys::path::append(IncludedFileName2, IncDir, "resp2");
   FS.addFile(IncludedFileName2, 0,
  MemoryBuffer::getMemBuffer("-option_21 -option_22\n"
-"-option_23=abcd\n"));
+"-option_23=abcd\n"
+"-option_24=<@>\n"));
 
   // Create second included response file of second level.
   llvm::SmallString<128> IncludedFileName3;
   llvm::sys::path::append(IncludedFileName3, IncDir, "resp3");
   FS.addFile(IncludedFileName3, 0,
  MemoryBuffer::getMemBuffer("-option_31 -option_32\n"
-"-option_33=abcd\n"));
+"-option_33=abcd\n"
+"-option_34=<@>\n"));
 
   // Prepare 'file' with reference to response file.
   SmallString<128> IncRef;
@@ -828,13 +832,21 @@
   llvm::StringSaver Saver(A);
   ASSERT_TRUE(llvm::cl::ExpandResponseFiles(
   Saver, llvm::cl::TokenizeGNUCommandLine, Argv, false, true,
-  /*CurrentDir=*/StringRef(TestRoot), FS));
-  EXPECT_THAT(Argv, testing::Pointwise(
-StringEquality(),
-{"test/test", "-flag_1", "-option_1", "-option_2",
- "-option_21", "-option_22", "-option_23=abcd",
- "-option_3=abcd", "-option_31", "-option_32",
- "-option_33=abcd", "-option_4=efjk", "-flag_2"}));
+  /*CurrentDir=*/TestRoot, FS));
+
+  std::string ExpectedOption24 = ("-option_24=" + TestRoot + "incdir").str();
+  std::string ExpectedOption34 = ("-option_34=" + TestRoot + "incdir").str();
+  std::string ExpectedOption5 = ("-option_5=" + TestRoot).str();
+  std::string ExpectedOption6 = ("-option_6=" + TestRoot + "sub").str();
+
+  EXPECT_THAT(
+  Argv, testing::Pointwise(
+StringEquality(),
+{"test/test", "-flag_1", "-option_1", "-option_2", "-option_21",
+ "-option_22", "-option_23=abcd", ExpectedOption24.c_str(),
+ "-option_3=abcd", "-option_31", "-option_32",
+ "-option_33=abcd", ExpectedOption34.c_str(), "-option_4=efjk",
+ ExpectedOption5.c_str(), ExpectedOption6.c_str(), "-flag_2"}));
 }
 
 TEST(CommandLineTest, RecursiveResponseFiles) {
@@ -1045,15 +1057,17 @@
   TempFile ConfigFile(TestCfg, "",
   "# Comment\n"
   "-option_1\n"
+  "-option_2=<@>/dir1\n"
   "@subconfig\n"
-  "-option_3=abcd\n"
-  "-option_4=\\\n"
+  "-option_5=abcd\n"
+  "-option_6=\\\n"
   "cdef\n");
 
   llvm::SmallString<128> TestCfg2;
   llvm::sys::path::append(TestCfg2, TestDir.path(), "subconfig");
   TempFile ConfigFile2(TestCfg2, "",
-   "-option_2\n"
+   "-option_3\n"
+   "-option_4=<@>/dir2\n"
"\n"
"   # comment\n");
 
@@ -1071,11 +1085,15 @@
   bool Result = llvm::cl::readConfigFile(ConfigFile.path(), Saver, Argv);
 
   EXPECT_TRUE(Result);
-  EXPECT_EQ(Argv.size(), 4U);
+  EXPECT_EQ(Argv.size(), 6U);
   EXPECT_STREQ(Argv[0], "-option_1");
-  EXPECT_STREQ(Argv[1], "-option_2");
-  

[PATCH] D115604: [Support] Expand `<@>` as the base directory in response files.

[Jack Andersen via Phabricator via cfe-commits]

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

Add entry to user manual explaining the `<@>` token.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115604

Files:
  clang/docs/UsersManual.rst
  llvm/lib/Support/CommandLine.cpp
  llvm/unittests/Support/CommandLineTest.cpp

Index: llvm/unittests/Support/CommandLineTest.cpp
===
--- llvm/unittests/Support/CommandLineTest.cpp
+++ llvm/unittests/Support/CommandLineTest.cpp
@@ -796,8 +796,10 @@
  llvm::MemoryBuffer::getMemBuffer("-option_1 -option_2\n"
   "@incdir/resp2\n"
   "-option_3=abcd\n"
-  "@incdir/resp3\n"
-  "-option_4=efjk\n"));
+  "@<@>/incdir/resp3\n"
+  "-option_4=efjk\n"
+  "-option_5=<@>\n"
+  "-option_6=<@>/sub\n"));
 
   // Directory for included file.
   llvm::StringRef IncDir = "incdir";
@@ -807,14 +809,16 @@
   llvm::sys::path::append(IncludedFileName2, IncDir, "resp2");
   FS.addFile(IncludedFileName2, 0,
  MemoryBuffer::getMemBuffer("-option_21 -option_22\n"
-"-option_23=abcd\n"));
+"-option_23=abcd\n"
+"-option_24=<@>\n"));
 
   // Create second included response file of second level.
   llvm::SmallString<128> IncludedFileName3;
   llvm::sys::path::append(IncludedFileName3, IncDir, "resp3");
   FS.addFile(IncludedFileName3, 0,
  MemoryBuffer::getMemBuffer("-option_31 -option_32\n"
-"-option_33=abcd\n"));
+"-option_33=abcd\n"
+"-option_34=<@>\n"));
 
   // Prepare 'file' with reference to response file.
   SmallString<128> IncRef;
@@ -829,12 +833,14 @@
   ASSERT_TRUE(llvm::cl::ExpandResponseFiles(
   Saver, llvm::cl::TokenizeGNUCommandLine, Argv, false, true,
   /*CurrentDir=*/StringRef(TestRoot), FS));
-  EXPECT_THAT(Argv, testing::Pointwise(
-StringEquality(),
-{"test/test", "-flag_1", "-option_1", "-option_2",
- "-option_21", "-option_22", "-option_23=abcd",
- "-option_3=abcd", "-option_31", "-option_32",
- "-option_33=abcd", "-option_4=efjk", "-flag_2"}));
+  EXPECT_THAT(
+  Argv, testing::Pointwise(
+StringEquality(),
+{"test/test", "-flag_1", "-option_1", "-option_2", "-option_21",
+ "-option_22", "-option_23=abcd", "-option_24=/incdir",
+ "-option_3=abcd", "-option_31", "-option_32",
+ "-option_33=abcd", "-option_34=/incdir", "-option_4=efjk",
+ "-option_5=/", "-option_6=/sub", "-flag_2"}));
 }
 
 TEST(CommandLineTest, RecursiveResponseFiles) {
@@ -1045,15 +1051,17 @@
   TempFile ConfigFile(TestCfg, "",
   "# Comment\n"
   "-option_1\n"
+  "-option_2=<@>/dir1\n"
   "@subconfig\n"
-  "-option_3=abcd\n"
-  "-option_4=\\\n"
+  "-option_5=abcd\n"
+  "-option_6=\\\n"
   "cdef\n");
 
   llvm::SmallString<128> TestCfg2;
   llvm::sys::path::append(TestCfg2, TestDir.path(), "subconfig");
   TempFile ConfigFile2(TestCfg2, "",
-   "-option_2\n"
+   "-option_3\n"
+   "-option_4=<@>/dir2\n"
"\n"
"   # comment\n");
 
@@ -1071,11 +1079,15 @@
   bool Result = llvm::cl::readConfigFile(ConfigFile.path(), Saver, Argv);
 
   EXPECT_TRUE(Result);
-  EXPECT_EQ(Argv.size(), 4U);
+  EXPECT_EQ(Argv.size(), 6U);
   EXPECT_STREQ(Argv[0], "-option_1");
-  EXPECT_STREQ(Argv[1], "-option_2");
-  EXPECT_STREQ(Argv[2], "-option_3=abcd");
-  EXPECT_STREQ(Argv[3], "-option_4=cdef");
+  EXPECT_STREQ(Argv[1],
+   ("-option_2=" + TestDir.path() + "/dir1").str().c_str());
+  EXPECT_STREQ(Argv[2], "-option_3");
+  EXPECT_STREQ(Argv[3],
+   ("-option_4=" + TestDir.path() + "/dir2").str().c_str());
+  EXPECT_STREQ(Argv[4], "-option_5=abcd");
+  EXPECT_STREQ(Argv[5], "-option_6=cdef");
 }
 
 TEST(CommandLineTest, PositionalEatArgsError) {
Index: llvm/lib/Support/CommandLine.cpp
===
--- llvm/lib/Support/CommandLine.cpp