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

[Alexandre Ganea via Phabricator via cfe-commits]

aganea 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;
+
----------------
hans wrote:
> 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.
Reid said "pack" and "field" and my Pavlovian reflex made me think "bitfields". Yes, plain bools are just fine.


================
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();
----------------
hans wrote:
> 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?
"burying", as in https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/Support/BuryPointer.h

Changed the comment and the function to `enableFree()`.



================
Comment at: clang/lib/Driver/Job.cpp:325
+  llvm::erase_if(Arguments, RemoveDisableFree);
+}
+
----------------
hans wrote:
> 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.
That was my original intent, but the `-disable-free` flag is added by `Clang::ConstructJob()` and at that point we don't know yet how many jobs we will have, nor their nature.


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

https://reviews.llvm.org/D74447