[flang-commits] [PATCH] D124846: [flang][driver] Support reading response files

[Andrzej Warzynski via Phabricator via flang-commits]

awarzynski added a comment.

In D124846#3493556 <https://reviews.llvm.org/D124846#3493556>, @rovka wrote:

> - Added a note about the @ syntax to the commit message (I hope that's what you had in mind, if not let me know!)

Your comment is perfect, thanks!

> - Removed the comments about writing response files. It seems we already support that thanks to the common clang infrastructure and this line <https://github.com/llvm/llvm-project/blob/2c14cdf831b677063a6518904b765c1f08d8557b/clang/lib/Driver/ToolChains/Flang.cpp#L136>. Yay \o/

Haha, it's been there since before `flang-new` existed 😂 !



================
Comment at: flang/test/Driver/response-file.f90:4
+! RUN: echo "-DTEST" > %basename_t.rsp
+! RUN: %flang -E -cpp @%basename_t.rsp %s -o - | FileCheck %s
+! RUN: %flang_fc1 -E -cpp @%basename_t.rsp %s -o - | FileCheck %s
----------------
rovka wrote:
> awarzynski wrote:
> > What happens if you skip `@`? Perhaps test for that too.
> We get a 'linker input unused' warning (same as clang). Not sure if that's really worth testing...
Perhaps the actual error is not relevant, but this would be nice:
```
! Works fine - valid syntax
! RUN: %flang -E -cpp @%basename_t.rsp %s -o - | FileCheck %s
! Fails - invalid syntax
! RUN: not %flang -E -cpp %basename_t.rsp %s -o - | FileCheck %s
```


================
Comment at: flang/tools/flang-driver/driver.cpp:79
 
+  // Parse response files. We're defaulting to the GNU syntax, since we don't
+  // have a CL mode.
----------------
rovka wrote:
> awarzynski wrote:
> > Would it make sense to move this to a dedicated function? I imagine that this might grow in the future (especially when working on Windows). No strong opinion.
> Can do.
Thanks for the update! I was thinking about something that would hide all the implementation details of dealing with response files from `main`, e.g.:
```
ProcessResponseFiles(args);
```

Basically, somebody scanning `main` does not need to know/care about response files at all, right? The most important part is that `args` is being updated with the contents of response files.

Perhaps I'm a bit picky, but to me response files are still something exotic and niche and hence I'm keen to make this very clear :) 

This is not a blocker for me.


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

https://reviews.llvm.org/D124846