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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
