[PATCH] D39502: [Driver] Make clang/cc conforms to UNIX standard

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

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