hans added a comment.
Thanks! This is starting to look really good now.
================
Comment at: clang/lib/Driver/Job.cpp:318
-int Command::Execute(ArrayRef<llvm::Optional<StringRef>> Redirects,
- std::string *ErrMsg, bool *ExecutionFailed) const {
+int Command::PrepareExecution(SmallVectorImpl<const char *> &Argv,
+ std::string *ErrMsg,
----------------
Maybe better to just return a bool for success/failure since there is not
really a process exit code yet.
(But I'm not even sure this is worth a separate method; see comment further
down.)
================
Comment at: clang/lib/Driver/Job.cpp:386
+ SmallVector<const char *, 128> Argv;
+ int R = PrepareExecution(Argv, ErrMsg, ExecutionFailed);
+ if (R)
----------------
For CC1Command, PrepareExecution doesn't do very much (since it doesn't use
response files). Because of that, I'm not sure breaking out PrepareExecution
into a separate method is worth it. I'd just copy the part that applies to
CC1Command here. Then we wouldn't need to check the return value from
PrepareExecution (it can't fail if response files aren't used), and we also
don't need to intercept SetResponsefile -- we'd just ignore it even if it's set.
================
Comment at: clang/lib/Driver/Job.cpp:415
+ // We don't support set a new environment when calling into ExecuteCC1Tool()
+ assert(0 && "The CC1Command doesn't support changing the environment vars!");
+}
----------------
llvm_unreachable("msg") is usually preferred instead of assert(0 && "msg")
================
Comment at: clang/tools/driver/driver.cpp:313
+ llvm::cl::ResetAllOptionOccurrences();
+ StringRef Tool = argv[1] + 4;
void *GetExecutablePathVP = (void *)(intptr_t) GetExecutablePath;
----------------
This feels a little risky to me. Do we still need to rely on argv[1], now that
we have a nice CC1Command abstraction?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69825/new/
https://reviews.llvm.org/D69825
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
