[PATCH] D95793: [clang][cli] Generate and round-trip language options

[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.

LGTM, since I don't think the parallel error paths are too bad in this specific patch, but see my longer comment inline.



================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1196
                                 const std::vector<std::string> &Sanitizers,
                                 DiagnosticsEngine &Diags, SanitizerSet &S) {
+  bool Success = true;
----------------
jansvoboda11 wrote:
> dexonsmith wrote:
> > Can the caller use `Diags.hasErrorOccurred()` to avoid this change?
> Not really, because the caller might have `Diags` that already contains some errors. We could compare the number of errors in `Diags` before and after calling this function. `DiagnosticsEngine` doesn't have API for that, but that could be added without much issues.
> 
> Do you have any specific reason for avoiding this change beyond minimizing the diff?
> Not really, because the caller might have Diags that already contains some errors. We could compare the number of errors in Diags before and after calling this function. DiagnosticsEngine doesn't have API for that, but that could be added without much issues.

Why not just return "failure" at the top level if any error has occurred? Why do we need to know if a specific parse routine failed?

If we have top-level callers that send in a DiagnosticEngine that has already been called, maybe we can change them to call `Reset` before entering.

> Do you have any specific reason for avoiding this change beyond minimizing the diff? 

It's just fragile; it's a manual step to update Success every time an error is reported. Having two paths to give the same information is awkward and error-prone.

It'd be nice to have a hook that did both, so you could do something like:
```
reportError(diag::err_drv_invalid_value) << FlagName << Sanitizer;
```
and `reportError` would be something like:
```
auto reportError = [&Diags,&Success](OptSpecifier Opt) {
  Success = false;
  return Diags.Report(Opt);
};
```
or to ensure warnings in the same interface, maybe could have:
```
auto reportDiag = [&Diags,&Success](OptSpecifier Opt) {
  Success &= Diags.getDiagnosticLevel() >= DiagnosticsEngine::Error;
  return Diags.Report(Opt);
};
```
Not necessarily worth it for this patch (you can commit as-is), but if we're going to have to update swaths of code in this way it would be good to encapsulate the pattern somehow.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95793