Add a RangeSpecificDiagnosticConsumer #14844
Conversation
|
@swift-ci Please smoke test |
lib/AST/DiagnosticConsumer.cpp
Outdated
| StringRef FormatString, ArrayRef<DiagnosticArgument> FormatArgs, | ||
| const DiagnosticInfo &Info) { | ||
| if (Kind != DiagnosticKind::Note) { | ||
| // When emitting a new top-level diagnostic, figure out if it belongs to |
There was a problem hiding this comment.
My style would be to split out the function that finds a containing buffer (or the CurrentGroupConsumer) from the function that handles the diagnostic. The latter would call the former to get the buffer (consumer) (if any). It would read a tad more clearly that way to my eyes.
There was a problem hiding this comment.
As I get further into this code, I would like handleDiagnostic to be something like this:
[There would be a static function in this class called shouldResetCurrentGroupConsumer that would take a diagnostic as a parameter and return true iff not a note.]
This function would only say:
if (shouldResetCurrentGroupConsumer(Kind)) {
CurrentGroupConsumer = findConsumerFor(Loc);
}
consumeDiagnostic(CurrentGroupConsumer, SM, Loc, Kind, FormatString, FormatArgs, Info)
The idea is to more clearly manifest the two-step nature of the process.
Another option would be to have a function that returned a vector consisting of either all the SubConsumers or just the one appropriate SubConsumer for that diagnostic. That would eliminate the if-else with the duplicated call to handleDiagnostic. Or have a forAllRelevantConsumers function that took a closure and either replicated the diagnostic handling or not.
I guess I'm hoping to break this function up a bit to make it easier for eyes like mine to see the structure of the computation.
There was a problem hiding this comment.
Glad you like the idea. Tx.
lib/AST/DiagnosticConsumer.cpp
Outdated
|
|
||
| FileSpecificDiagnosticConsumer::FileSpecificDiagnosticConsumer( | ||
| MutableArrayRef<ConsumerPair> consumers) { | ||
| for (ConsumerPair &pair : consumers) { |
There was a problem hiding this comment.
My style would be to extract out the body of this constructor into a function called something like buildSortedConsumerMap. That function would in turn call populateMap then sortMap. Even though the code is clear as is, I like to make it even easier to read quickly.
There was a problem hiding this comment.
I don't think this one pulls its weight. I added comments, though.
| SmallVector<std::unique_ptr<DiagnosticConsumer>, 4> SubConsumers; | ||
| SmallVector<std::pair<CharSourceRange, unsigned>, 4> LocationToConsumerMap; | ||
| static const unsigned NO_SPECIFIC_CONSUMER = -1; | ||
| unsigned CurrentGroupConsumer = NO_SPECIFIC_CONSUMER; |
There was a problem hiding this comment.
Would this work as a local variable inside of handleDiagnostic? I don't see other uses of it, and AFAICT it is set each time that routine is called.
There was a problem hiding this comment.
Ah--now I see it is set for non-notes and reused for notes. A better name would be GroupConsumerForPreviousNonNote. No, that's not quite right. At least a Doxygen comment to explain it would help.
There was a problem hiding this comment.
Glad you like the idea. Tx.
| topConsumer.finishProcessing(); | ||
| } | ||
|
|
||
| TEST(FileSpecificDiagnosticConsumer, CrossFileNotes) { |
There was a problem hiding this comment.
I find these test routines a bit daunting. Would it make sense to add comments and/or break them up a bit? I'm thinking of a comment or subroutine name like: "Ensure that a Note ends up in the same consumer as the most recent non-note." Or some such, so a reader can gather the intention without delving into the code.
Another approach: list the ways you can imagine it failing, and test each one. That's probably what this is, but it's hard to tell at a glance.
At a glance, there might be a factoring that could make these test routines a lot more concise. Just a thought...
There was a problem hiding this comment.
I admit to testing multiple things in each function, since the setup is so clumsy. But ownership and having all the expectations up front makes it hard to make the setup not clumsy. Maybe making the diagnostic emission less noisy would help, so you could compare the expected per-file output with what's going to be emitted?
There was a problem hiding this comment.
No, it turns out I can come up with reasonable names for all these tests.
If a top-level diagnostic is in a particular source range, the RangeSpecificDiagnosticConsumer will funnel it and any attached notes to a particular "sub-consumer" designated for that range (intended to be used with whole files). If it's not in a range designated for any sub-consumer, the diagnostic is passed to all registered sub-consumers. This is intended to be used for batch mode, so that diagnostics that are definitely associated with a particular file can be emitted in that file's .dia output, while diagnostics that may be associated with other files (such as those that come from Clang) will still get presented to the user.
jrose-apple
force-pushed
the
FileSpecificDiagnosticConsumer
branch
from
a546ae3 to
54867c0
Compare
|
@swift-ci Please smoke test |
|
Build failed |
|
Build failed |
jrose-apple
changed the title
|
@davidungar: Still waiting for another round of review, or you could jump to #14900 and see how it's used (and the unfortunate but unavoidable switch to basing on file names). |
|
(We moved to just doing it all at once in #14900. Closing.) |
If a top-level diagnostic is in a particular source range, the RangeSpecificDiagnosticConsumer will funnel it and any attached notes to a particular "sub-consumer" designated for that range (intended to be used with whole files). If it's not in a range designated for any sub-consumer, the diagnostic is passed to all registered sub-consumers.
This is intended to be used for batch mode, so that diagnostics that are definitely associated with a particular file can be emitted in that file's .dia output, while diagnostics that may be associated with other files (such as those that come from Clang) will still get presented to the user.