[PATCH] D74447: [Clang] After integrated-cc1, ignore -disable-free when there are more than one job in the queue

[Hans Wennborg via Phabricator via cfe-commits]

hans added inline comments.


================
Comment at: clang/include/clang/Driver/Job.h:90
+  /// Whether the command will be executed in this process or not.
+  bool InProcess : 1;
+
----------------
I think Reid just meant put the bool fields after each other to minimize padding. Using bitfields is taking it one step further. I think in LLVM we generally use "unsigned" for bitfield types, but I'm not sure using bitfields at all is worth it here.


================
Comment at: clang/include/clang/Driver/Job.h:134
 
-  /// Set whether to print the input filenames when executing.
-  void setPrintInputFilenames(bool P) { PrintInputFilenames = P; }
+  /// Prevent burying pointers, and ensure we free everything after execution.
+  void forceCleanUp();
----------------
I hadn't heard the term "burying pointers" before, and the name is also pretty broad: "clean up" could refer to a lot more than just freeing memory.

Maybe "enableFree()" or something would be a better name?


================
Comment at: clang/lib/Driver/Driver.cpp:3758
+  JobList &Jobs = C.getJobs();
+  if (!Jobs.empty()) {
+    Command &Last = *Jobs.getJobs().back();
----------------
Is this if-statement really necessary? If you just do the for-loop directly, it will not execute if Jobs is empty anyway.

Also, with an old-school for-loop it's easier to not iterate over the last element. How about something like:

```
for (size_t i = 0, e = C.getJobs().size(); i + 1 < e; i++) {
  auto &Job = C.getJobs().getJobs()[i];
  if (Job.InProcess)
    Job.forceCleanup();
}
```


================
Comment at: clang/lib/Driver/Job.cpp:325
+  llvm::erase_if(Arguments, RemoveDisableFree);
+}
+
----------------
I wish it were possible to avoid adding the -disable-free flag to in-process-cc1 jobs in the first place, but I guess maybe that's not practical.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74447/new/

https://reviews.llvm.org/D74447