[PATCH] D69825: [Clang][Driver] Re-use the calling process instead of creating a new process for the cc1 invocation

[Hans Wennborg via Phabricator via llvm-commits]

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