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

[Alexandre Ganea via Phabricator via llvm-commits]

aganea accepted this revision.
aganea added a comment.

LGTM.



================
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);
----------------
mstorsjo wrote:
> aganea wrote:
> > 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?
> With `Expected<>` I'd need to craft an `Error` at the end if I don't have a path to return, but do you mean `Optional<>`? I guess that'd work - but as `findProgramByName()` returns `ErrorOr<std::string>` I kept the same signature.
> 
> Even if returning `Optional<>`, we need a local variable to receive the `ErrorOr<std::string>` from `findProgramByName()` and inspect it.
> 
> In the future (after a release or so) I'd intend for this to be a hard error, so at that point, the returned error code actually would be printed.
I see, thanks for the explanation!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100755