[PATCH] D108918: [clang] NFC: Extract DiagnosticOptions parsing

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

dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.

A few comments inline, but LGTM once you fix those.



================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:2259-2260
 
+DiagnosticOptions *clang::CreateAndPopulateDiagOpts(ArrayRef<const char *> Argv,
+                                                    bool *UseNewCC1Process) {
+  auto *DiagOpts = new DiagnosticOptions;
----------------
Please return `std::unique_ptr<DiagnosticOptions` if this is an owning pointer. The caller can call `.release` if it needs to unpack it (although the current caller needn't do anything since there's an implicit conversion to `IntrusiveRefCntPtr` from `std::unique_ptr`).

Also, I'd prefer to propagate the lowerCamelCase style for functions recommended by the coding standards... up to you I guess since I know this file has a lot of UpperCamelCase.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:2261
+                                                    bool *UseNewCC1Process) {
+  auto *DiagOpts = new DiagnosticOptions;
+  unsigned MissingArgIndex, MissingArgCount;
----------------
Please use `std::make_unique<DiagnosticOptions>()` instead of `new`.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:2270-2274
+  if (UseNewCC1Process)
+    *UseNewCC1Process =
+        Args.hasFlag(clang::driver::options::OPT_fno_integrated_cc1,
+                     clang::driver::options::OPT_fintegrated_cc1,
+                     /*Default=*/CLANG_SPAWN_CC1);
----------------
I don't think `-fintegrated-cc1` is related to diagnostic options. I suggest leaving it behind in the driver code and using:
```
lang=c++
std::unique_ptr<DiagnosticOptions>
clang::createAndPopulateDiagOpts(ArrayRef<const char *> Argv) {
  return createAndPopulateDiagOptsImpl(Argv);
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108918