dexonsmith added a comment. I have a few nitpicks, but otherwise this LGTM. I'd like to wait for someone on the CUDA side to confirm though.
================ Comment at: lib/Driver/Compilation.cpp:185 -void Compilation::ExecuteJobs( - const JobList &Jobs, - SmallVectorImpl<std::pair<int, const Command *>> &FailingCommands) const { +typedef SmallVectorImpl< std::pair<int, const Command *> > FailingCommandList; + ---------------- Have you run clang-format-diff.py on your patch? There's some funny whitespace here. Also, you might consider splitting this out in a separate NFC commit. ================ Comment at: lib/Driver/Compilation.cpp:193-196 + for (FailingCommandList::const_iterator CI = FailingCommands.begin(), + CE = FailingCommands.end(); CI != CE; ++CI) + if (A == &(CI->second->getSource())) + return true; ---------------- Ranged-based `for` seems better here: ``` for (const auto &CI : FailingCommands) if (A == &CI.second->getSource()) return true; ``` ================ Comment at: lib/Driver/Compilation.cpp:218 FailingCommands.push_back(std::make_pair(Res, FailingCommand)); - // Bail as soon as one command fails, so we don't output duplicate error - // messages if we die on e.g. the same file. - return; + // Bail on error in cl mode. + if (TheDriver.IsCLMode()) ---------------- I think the longer comment was useful; just tweak it to be cl-mode-specific. ================ Comment at: test/Driver/output-file-cleanup.c:53-58 // RUN: not %clang -S %t-dir/1.c %t-dir/2.c %t-dir/3.c %t-dir/4.c %t-dir/5.c // RUN: test -f %t-dir/1.s // RUN: test ! -f %t-dir/2.s -// RUN: test ! -f %t-dir/3.s +// RUN: test -f %t-dir/3.s // RUN: test ! -f %t-dir/4.s +// RUN: test -f %t-dir/5.s ---------------- IIRC, the tests were like this before, and then they were changed as convenient when the CUDA code was introduced. I suggest adding a comment around here explaining *why* we expect 3.s and 5.s to exist (i.e., for UNIX conformance of the cc/c89/c99 aliases) so that the behaviour isn't changed again in the future without careful consideration. https://reviews.llvm.org/D39502 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits