[PATCH] D100755: [llvm-rc] [3/4] Run clang to preprocess input files

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added inline comments.


================
Comment at: llvm/tools/llvm-rc/llvm-rc.cpp:94
+
+ErrorOr<std::string> findClang(const char *Argv0) {
+  StringRef Parent = llvm::sys::path::parent_path(Argv0);
----------------
Since you're not using the `std::error_code` below in the call site, should this return `Expected<...>`? In that case, the variable `Path` shouldn't be needed?


================
Comment at: llvm/tools/llvm-rc/llvm-rc.cpp:154
+      Args.push_back("-U");
+      break;
+    }
----------------
thakis wrote:
> mstorsjo wrote:
> > thakis wrote:
> > > Here's our chromium wrapper: https://source.chromium.org/chromium/chromium/src/+/master:build/toolchain/win/rc/rc.py
> > > 
> > > On Windows, /winsysroot support and possibly -imsvc support would be nice too.
> > Those sound useful - but I think I'd prefer to defer adding support for more nonstandard preprocessing options to a later patch.
> > 
> > What do you think of a generic `--preprocessor-arg` like in the windres frontend, which might be useful for @aganea?
> We don't need a general `--preprocessor-arg` as long as common args work.
> 
> Oh, on that note: https://source.chromium.org/chromium/chromium/src/+/master:build/toolchain/win/rc/rc.py also has `/showIncludes` support which is necessary to make ninja re-build .rc files if either
> 
> * an included .h file is changed (needs preprocessor output)
> * an included resource file (.ico or similar) is changed (needs llvm-rc support)
> 
> Don't know if llvm-rc has support for the latter part. If it doesn't, that'd be nice to add, and some test that checks that both parts work would be nice. (Neither in this patch; just thinking about what we'd need to switch.)
> We don't need a general --preprocessor-arg as long as common args work.
+1 with Nico. Native arguments in `llvm-rc` would be better (later).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100755