Page MenuHomePhabricator

[clang][cli] NFC: Ensure non-null DiagnosticsEngine in ParseDiagnosticArgs
ClosedPublic

Authored by jansvoboda11 on Dec 22 2020, 6:49 AM.

Details

Summary

Before this patch, ParseDiagnosticArgs can be called with a nullptr DiagnosticsEngine *. This happens early on in the compilation process, where no proper DiagnosticEngine exists, because the diagnostic options (passed through command line) are not known yet.

This patch ensures nullptr is replaced by an ignoring DiagnosticEngine in ParseDiagnosticArgs, which allows to switch from pointer to a reference in some utility functions.

Besides simplifying the code, this patch enables a future patch (D84673) that ports diagnostic options to the new marshalling infrastructure.

Diff Detail

Event Timeline

jansvoboda11 requested review of this revision.Dec 22 2020, 6:49 AM
jansvoboda11 created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptDec 22 2020, 6:49 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

This is needed for a future patch, where we start using normalizers in contexts where no Diags are available.

Can you explain what those contexts are? I'm wondering if they can be changed to create a DiagnosticsEngine instead of having to account for it being maybe missing in all these places.

This will make more sense after looking at D84673. In short: parsing of diagnostic options can happen outside of CompilerInvocation::createFromArgs, that's why ParseDiagnosticArgs is a free function. Ideally, we'd like to reuse the existing marshalling infrastructure here as well.

ParseDiagnosticArgs is usually called with DiagnosticEngine *Diags = nullptr. That's because the call happens early on during compiler execution, where we don't have a diagnostic engine yet. This call needs to happen first, to obtain the diagnostic options that are necessary for the engine instantiation.

This patch accommodates this free function by turning all DiagnosticEngine references to pointers. Another solution would be to create a "dummy" DiagnosticsEngine for the ParseDiagnosticArgs calls, but I didn't find a way to do that.

Another solution would be to create a "dummy" DiagnosticsEngine for the ParseDiagnosticArgs calls, but I didn't find a way to do that.

Something like CompilerInstance::createDiagnostics(new DiagnosticOptions(), new IgnoreDiagnostics()) could work. I'll give that a try.

Another solution would be to create a "dummy" DiagnosticsEngine for the ParseDiagnosticArgs calls, but I didn't find a way to do that.

Something like CompilerInstance::createDiagnostics(new DiagnosticOptions(), new IgnoreDiagnostics()) could work. I'll give that a try.

I think you can also construct a DiagnosticsEngine directly on the stack, without going through CompilerInstance, if that helps.

This will make more sense after looking at D84673. In short: parsing of diagnostic options can happen outside of CompilerInvocation::createFromArgs, that's why ParseDiagnosticArgs is a free function. Ideally, we'd like to reuse the existing marshalling infrastructure here as well.

ParseDiagnosticArgs is usually called with DiagnosticEngine *Diags = nullptr. That's because the call happens early on during compiler execution, where we don't have a diagnostic engine yet. This call needs to happen first, to obtain the diagnostic options that are necessary for the engine instantiation.

This patch accommodates this free function by turning all DiagnosticEngine references to pointers. Another solution would be to create a "dummy" DiagnosticsEngine for the ParseDiagnosticArgs calls, but I didn't find a way to do that.

Also, please try put this level of detail in the commit message.

Instead of making Diags optional everywhere, pass an ignoring Diags into ParseDiagnosticArgs.

jansvoboda11 retitled this revision from [clang][cli] NFC: Make Diags optional in normalizer to [clang][cli] NFC: Pass an ignoring DiagnosticsEngine instead of nullptr to ParseDiagnosticArgs.Jan 6 2021, 5:28 AM
jansvoboda11 edited the summary of this revision. (Show Details)
dexonsmith added inline comments.Jan 6 2021, 12:41 PM
clang/lib/Frontend/CompilerInvocation.cpp
1405–1407

Instead of this change (and updating callers), I suggest adding:

Optional<DiagnosticsEngine> IgnoredDiags;
if (!Diags) {
  IgnoredDiags.emplace(new DiagnosticIDs(), new DiagnosticOptions(),
                       new IgnoringDiagConsumer());
  Diags = &*IgnoredDiags;
}
dexonsmith added a comment.EditedJan 6 2021, 12:43 PM

With the above adjustment (reverting the change to the ParseDiagnosticArgs signature), this LGTM.

dexonsmith accepted this revision.Jan 6 2021, 12:43 PM
This revision is now accepted and ready to land.Jan 6 2021, 12:43 PM
jansvoboda11 retitled this revision from [clang][cli] NFC: Pass an ignoring DiagnosticsEngine instead of nullptr to ParseDiagnosticArgs to [clang][cli] NFC: Ensure non-null DiagnosticsEngine in ParseDiagnosticArgs.Jan 7 2021, 4:56 AM
jansvoboda11 edited the summary of this revision. (Show Details)
jansvoboda11 added inline comments.Jan 7 2021, 5:01 AM
clang/lib/Frontend/CompilerInvocation.cpp
1405–1407

That's a nice way to keep the signature intact. Thanks!