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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
