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