Meinersbur added inline comments.

================
Comment at: clang/lib/Driver/Job.cpp:340-355
+  typedef int (*ClangDriverMainFunc)(SmallVectorImpl<const char *> &);
+  ClangDriverMainFunc ClangDriverMain = nullptr;
+
+  if (ReenterTool) {
+    StringRef F = llvm::sys::path::filename(Executable);
+    if (F.endswith_lower(".exe"))
+      F = F.drop_back(4);
----------------
Meinersbur wrote:
> This looks fragile as it will break when the user chooses to rename the 
> executable (`clang-cuda`, `--driver-mode=...`,...). Why not moving the 
> ClangDriverMain logic into the clangDriver module. Or, at least, pass it 
> through a lambda. It would also make it easier to use clangDriver as a 
> library (if that was ever an intended use-case?!?)
I tried this on Linux and it did not work out-of-the box (`ClangDriverMain` 
being nullptr) due to the executable's name being `clang-10`. After fixing, 
`check-clang` failed with 5 errors in the Driver test.

The performance benefit was within noise: 6m20s vs. 6m18s on a 2-Socket 28 
cores-per-processor (112 SMT threads) Skylake-Xeon system for `ninja all 
check-clang`, assertions enabled.


Repository:
  rG LLVM Github Monorepo

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

Reply via email to