[PATCH] D136090: Handle errors in expansion of response files

[Serge Pavlov via Phabricator via llvm-commits]

sepavloff added inline comments.


================
Comment at: llvm/lib/Support/CommandLine.cpp:1188
+  // macros.
+  if (!RelativeNames && !InConfigFile)
     return Error::success();
----------------
rovka wrote:
> Why do you need to add `!InConfigFile` here and below? 
It is not necessary, because now configuration file is expanded with the flag `RelativeNames` set. It however seems more correct to check for `InConfigFile` also, hopefully it make code a bit clearly.


================
Comment at: llvm/lib/Support/CommandLine.cpp:1204
+    StringRef FileName;
+    if (ArgStr[0] == '@') {
+      FileName = ArgStr.drop_front(1);
----------------
rovka wrote:
> Nit: Why reverse this branch? The code around it `continue`s early, so it's easier to read if this `continue`s early too.
You are right. Initially this patch was combined with D136354, where additional checks was added. I changed this code to be more natural.


================
Comment at: llvm/lib/Support/CommandLine.cpp:1205
+    if (ArgStr[0] == '@') {
+      FileName = ArgStr.drop_front(1);
+      if (!llvm::sys::path::is_relative(FileName))
----------------
mgorny wrote:
> Also, I think you can use `consume_front()` here.
Yes, changed code accordingly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136090