Skip to content

Commit cd3c82d

Browse files
davidungardavidungar
committed
Addressing comments.
1 parent b253e15 commit cd3c82d

File tree

4 files changed

+58
-82
lines changed

4 files changed

+58
-82
lines changed

‎include/swift/AST/DiagnosticConsumer.h

Lines changed: 19 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -183,7 +183,7 @@ class FileSpecificDiagnosticConsumer : public DiagnosticConsumer {
183183
ArrayRef<DiagnosticArgument> FormatArgs,
184184
const DiagnosticInfo &Info) {
185185
if (!getConsumer())
186-
return; // Suppress non-primary diagnostic in batch mode.
186+
return;
187187
hasAnErrorBeenConsumed |= Kind == DiagnosticKind::Error;
188188
getConsumer()->handleDiagnostic(SM, Loc, Kind, FormatString, FormatArgs,
189189
Info);
@@ -200,35 +200,33 @@ class FileSpecificDiagnosticConsumer : public DiagnosticConsumer {
200200
SmallVector<Subconsumer, 4> Subconsumers;
201201

202202
public:
203-
// The commented-out consts are there because the data does not change
204-
// but the swap method gets called on this structure.
205-
class ConsumerSpecificInformation {
203+
class ConsumerAndRange {
206204
private:
207205
/// The range of SourceLoc's for which diagnostics should be directed to
208206
/// this subconsumer.
207+
/// Should be const but then the sort won't compile.
209208
/*const*/ CharSourceRange range;
210209

211210
/// Index into Subconsumers vector for this subconsumer.
211+
/// Should be const but then the sort won't compile.
212212
/*const*/ unsigned subconsumerIndex;
213-
213+
214214
public:
215-
ConsumerSpecificInformation(const CharSourceRange range,
215+
unsigned getSubconsumerIndex() const { return subconsumerIndex; }
216+
217+
ConsumerAndRange(const CharSourceRange range,
216218
unsigned subconsumerIndex)
217219
: range(range), subconsumerIndex(subconsumerIndex) {}
218220

219-
Subconsumer &subconsumer(FileSpecificDiagnosticConsumer &c) const {
220-
return c.Subconsumers[subconsumerIndex];
221-
}
222-
223221
/// Compare according to range:
224-
bool operator<(const ConsumerSpecificInformation &right) const {
222+
bool operator<(const ConsumerAndRange &right) const {
225223
auto compare = std::less<const char *>();
226224
return compare(getRawLoc(range.getEnd()).getPointer(),
227225
getRawLoc(right.range.getEnd()).getPointer());
228226
}
229227

230228
/// Overlaps by range:
231-
bool overlaps(const ConsumerSpecificInformation &other) const {
229+
bool overlaps(const ConsumerAndRange &other) const {
232230
return range.overlaps(other.range);
233231
}
234232

@@ -238,11 +236,14 @@ class FileSpecificDiagnosticConsumer : public DiagnosticConsumer {
238236
return compare(getRawLoc(range.getEnd()).getPointer(),
239237
getRawLoc(loc).getPointer());
240238
}
241-
239+
242240
bool contains(const SourceLoc loc) const { return range.contains(loc); }
243241
};
244242

245243
private:
244+
Subconsumer &operator[](const ConsumerAndRange& consumerAndRange) {
245+
return Subconsumers[consumerAndRange.getSubconsumerIndex()];
246+
}
246247
/// The consumers owned by this FileSpecificDiagnosticConsumer, sorted by
247248
/// the end locations of each file so that a lookup by position can be done
248249
/// using binary search.
@@ -251,8 +252,8 @@ class FileSpecificDiagnosticConsumer : public DiagnosticConsumer {
251252
/// This allows diagnostics to be emitted before files are actually opened,
252253
/// as long as they don't have source locations.
253254
///
254-
/// \see #consumerSpecificInformationForLocation
255-
SmallVector<ConsumerSpecificInformation, 4> ConsumersOrderedByRange;
255+
/// \see #consumerAndRangeForLocation
256+
SmallVector<ConsumerAndRange, 4> ConsumersOrderedByRange;
256257

257258
/// Indicates which consumer to send Note diagnostics too.
258259
///
@@ -261,7 +262,7 @@ class FileSpecificDiagnosticConsumer : public DiagnosticConsumer {
261262
///
262263
/// If None, Note diagnostics are sent to every consumer.
263264
/// If null, diagnostics are suppressed.
264-
Optional<ConsumerSpecificInformation *>
265+
Optional<ConsumerAndRange *>
265266
ConsumerSpecificInfoForSubsequentNotes = None;
266267

267268
bool HasAnErrorBeenConsumed = false;
@@ -294,8 +295,8 @@ class FileSpecificDiagnosticConsumer : public DiagnosticConsumer {
294295
/// Returns nullptr if diagnostic is to be suppressed,
295296
/// None if diagnostic is to be distributed to every consumer,
296297
/// a particular consumer if diagnostic goes there.
297-
Optional<ConsumerSpecificInformation *>
298-
consumerSpecificInformationForLocation(SourceManager &SM,
298+
Optional<ConsumerAndRange *>
299+
consumerAndRangeForLocation(SourceManager &SM,
299300
SourceLoc loc) const;
300301
};
301302

‎lib/AST/DiagnosticConsumer.cpp

Lines changed: 16 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -89,34 +89,30 @@ void FileSpecificDiagnosticConsumer::computeConsumersOrderedByRange(
8989
assert(bufferID.hasValue() && "consumer registered for unknown file");
9090
CharSourceRange range = SM.getRangeForBuffer(bufferID.getValue());
9191
ConsumersOrderedByRange.emplace_back(
92-
ConsumerSpecificInformation(range, subconsumerIndex));
92+
ConsumerAndRange(range, subconsumerIndex));
9393
}
9494

9595
// Sort the "map" by buffer /end/ location, for use with std::lower_bound
9696
// later. (Sorting by start location would produce the same sort, since the
9797
// ranges must not be overlapping, but since we need to check end locations
9898
// later it's consistent to sort by that here.)
99-
std::sort(ConsumersOrderedByRange.begin(), ConsumersOrderedByRange.end(),
100-
[](const ConsumerSpecificInformation &left,
101-
const ConsumerSpecificInformation &right) -> bool {
102-
return left < right;
103-
});
99+
std::sort(ConsumersOrderedByRange.begin(), ConsumersOrderedByRange.end());
104100

105101
// Check that the ranges are non-overlapping. If the files really are all
106102
// distinct, this should be trivially true, but if it's ever not we might end
107103
// up mis-filing diagnostics.
108104
assert(ConsumersOrderedByRange.end() ==
109105
std::adjacent_find(ConsumersOrderedByRange.begin(),
110106
ConsumersOrderedByRange.end(),
111-
[](const ConsumerSpecificInformation &left,
112-
const ConsumerSpecificInformation &right) {
107+
[](const ConsumerAndRange &left,
108+
const ConsumerAndRange &right) {
113109
return left.overlaps(right);
114110
}) &&
115111
"overlapping ranges despite having distinct files");
116112
}
117113

118-
Optional<FileSpecificDiagnosticConsumer::ConsumerSpecificInformation *>
119-
FileSpecificDiagnosticConsumer::consumerSpecificInformationForLocation(
114+
Optional<FileSpecificDiagnosticConsumer::ConsumerAndRange *>
115+
FileSpecificDiagnosticConsumer::consumerAndRangeForLocation(
120116
SourceManager &SM, SourceLoc loc) const {
121117
// Diagnostics with invalid locations always go to every consumer.
122118
if (loc.isInvalid())
@@ -150,16 +146,16 @@ FileSpecificDiagnosticConsumer::consumerSpecificInformationForLocation(
150146
// that /might/ contain 'loc'. Specifically, since the ranges are sorted
151147
// by end location, it's looking for the first range where the end location
152148
// is greater than or equal to 'loc'.
153-
const ConsumerSpecificInformation *possiblyContainingRangeIter =
149+
const ConsumerAndRange *possiblyContainingRangeIter =
154150
std::lower_bound(
155151
ConsumersOrderedByRange.begin(), ConsumersOrderedByRange.end(), loc,
156-
[](const ConsumerSpecificInformation &entry, SourceLoc loc) -> bool {
152+
[](const ConsumerAndRange &entry, SourceLoc loc) -> bool {
157153
return entry.endsAfter(loc);
158154
});
159155

160156
if (possiblyContainingRangeIter != ConsumersOrderedByRange.end() &&
161157
possiblyContainingRangeIter->contains(loc)) {
162-
return const_cast<ConsumerSpecificInformation *>(
158+
return const_cast<ConsumerAndRange *>(
163159
possiblyContainingRangeIter);
164160
}
165161

@@ -173,20 +169,20 @@ void FileSpecificDiagnosticConsumer::handleDiagnostic(
173169

174170
HasAnErrorBeenConsumed |= Kind == DiagnosticKind::Error;
175171

176-
Optional<ConsumerSpecificInformation *> consumerSpecificInfo;
172+
Optional<ConsumerAndRange *> consumerAndRange;
177173
switch (Kind) {
178174
case DiagnosticKind::Error:
179175
case DiagnosticKind::Warning:
180176
case DiagnosticKind::Remark:
181-
consumerSpecificInfo = consumerSpecificInformationForLocation(SM, Loc);
182-
ConsumerSpecificInfoForSubsequentNotes = consumerSpecificInfo;
177+
consumerAndRange = consumerAndRangeForLocation(SM, Loc);
178+
ConsumerSpecificInfoForSubsequentNotes = consumerAndRange;
183179
break;
184180
case DiagnosticKind::Note:
185-
consumerSpecificInfo = ConsumerSpecificInfoForSubsequentNotes;
181+
consumerAndRange = ConsumerSpecificInfoForSubsequentNotes;
186182
break;
187183
}
188-
if (consumerSpecificInfo.hasValue()) {
189-
consumerSpecificInfo.getValue()->subconsumer(*this).handleDiagnostic(SM,Loc, Kind, FormatString, FormatArgs, Info);
184+
if (consumerAndRange.hasValue()) {
185+
(*this)[*consumerAndRange.getValue()].handleDiagnostic(SM,Loc, Kind, FormatString, FormatArgs, Info);
190186
return;
191187
}
192188
for (auto &subconsumer : Subconsumers)
@@ -211,7 +207,7 @@ void FileSpecificDiagnosticConsumer::
211207
if (!HasAnErrorBeenConsumed)
212208
return;
213209
for (auto &info : ConsumersOrderedByRange)
214-
info.subconsumer(*this).informDriverOfIncompleteBatchModeCompilation();
210+
(*this)[info].informDriverOfIncompleteBatchModeCompilation();
215211
}
216212

217213
void NullDiagnosticConsumer::handleDiagnostic(

‎lib/FrontendTool/FrontendTool.cpp

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1586,7 +1586,7 @@ static std::unique_ptr<DiagnosticConsumer>
15861586
createDispatchingDiagnosticConsumerIfNeeded(
15871587
const FrontendInputsAndOutputs &inputsAndOutputs,
15881588
llvm::function_ref<std::unique_ptr<DiagnosticConsumer>(const InputFile &)>
1589-
maybeCreateSingleConsumer) {
1589+
maybeCreateConsumerForDiagnosticsFrom) {
15901590

15911591
// The "4" here is somewhat arbitrary. In practice we're going to have one
15921592
// sub-consumer for each diagnostic file we're trying to output, which (again
@@ -1600,9 +1600,8 @@ createDispatchingDiagnosticConsumerIfNeeded(
16001600

16011601
inputsAndOutputs.forEachInputProducingSupplementaryOutput(
16021602
[&](const InputFile &input) -> bool {
1603-
if (auto subconsumer = maybeCreateSingleConsumer(input))
1604-
subconsumers.emplace_back(FileSpecificDiagnosticConsumer::Subconsumer(
1605-
input.file(), std::move(subconsumer)));
1603+
if (auto subconsumer = maybeCreateConsumerForDiagnosticsFrom(input))
1604+
subconsumers.emplace_back(input.file(), std::move(subconsumer));
16061605
return false;
16071606
});
16081607
// For batch mode, the compiler must swallow diagnostics pertaining to

‎unittests/AST/DiagnosticConsumerTests.cpp

Lines changed: 20 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -64,10 +64,8 @@ TEST(FileSpecificDiagnosticConsumer, SubconsumersFinishInOrder) {
6464
consumerA.get(), None);
6565

6666
SmallVector<FileSpecificDiagnosticConsumer::Subconsumer, 2> consumers;
67-
consumers.emplace_back(
68-
FileSpecificDiagnosticConsumer::Subconsumer("A", std::move(consumerA)));
69-
consumers.emplace_back(FileSpecificDiagnosticConsumer::Subconsumer(
70-
"", std::move(consumerUnaffiliated)));
67+
consumers.emplace_back("A", std::move(consumerA));
68+
consumers.emplace_back("", std::move(consumerUnaffiliated));
7169

7270
auto topConsumer =
7371
FileSpecificDiagnosticConsumer::consolidateSubconsumers(consumers);
@@ -86,10 +84,8 @@ TEST(FileSpecificDiagnosticConsumer, InvalidLocDiagsGoToEveryConsumer) {
8684
consumerA.get(), expected);
8785

8886
SmallVector<FileSpecificDiagnosticConsumer::Subconsumer, 2> consumers;
89-
consumers.emplace_back(
90-
FileSpecificDiagnosticConsumer::Subconsumer("A", std::move(consumerA)));
91-
consumers.emplace_back(FileSpecificDiagnosticConsumer::Subconsumer(
92-
"", std::move(consumerUnaffiliated)));
87+
consumers.emplace_back("A", std::move(consumerA));
88+
consumers.emplace_back("", std::move(consumerUnaffiliated));
9389

9490
auto topConsumer =
9591
FileSpecificDiagnosticConsumer::consolidateSubconsumers(consumers);
@@ -129,10 +125,8 @@ TEST(FileSpecificDiagnosticConsumer, ErrorsWithLocationsGoToExpectedConsumers) {
129125
consumerA.get(), expectedB);
130126

131127
SmallVector<FileSpecificDiagnosticConsumer::Subconsumer, 2> consumers;
132-
consumers.emplace_back(
133-
FileSpecificDiagnosticConsumer::Subconsumer("A", std::move(consumerA)));
134-
consumers.emplace_back(
135-
FileSpecificDiagnosticConsumer::Subconsumer("B", std::move(consumerB)));
128+
consumers.emplace_back("A", std::move(consumerA));
129+
consumers.emplace_back("B", std::move(consumerB));
136130

137131
auto topConsumer =
138132
FileSpecificDiagnosticConsumer::consolidateSubconsumers(consumers);
@@ -186,10 +180,8 @@ TEST(FileSpecificDiagnosticConsumer,
186180
consumerA.get(), expectedUnaffiliated);
187181

188182
SmallVector<FileSpecificDiagnosticConsumer::Subconsumer, 2> consumers;
189-
consumers.emplace_back(
190-
FileSpecificDiagnosticConsumer::Subconsumer("A", std::move(consumerA)));
191-
consumers.emplace_back(FileSpecificDiagnosticConsumer::Subconsumer(
192-
"", std::move(consumerUnaffiliated)));
183+
consumers.emplace_back("A", std::move(consumerA));
184+
consumers.emplace_back("", std::move(consumerUnaffiliated));
193185

194186
auto topConsumer =
195187
FileSpecificDiagnosticConsumer::consolidateSubconsumers(consumers);
@@ -234,10 +226,8 @@ TEST(FileSpecificDiagnosticConsumer, WarningsAndRemarksAreTreatedLikeErrors) {
234226
consumerA.get(), expectedUnaffiliated);
235227

236228
SmallVector<FileSpecificDiagnosticConsumer::Subconsumer, 2> consumers;
237-
consumers.emplace_back(
238-
FileSpecificDiagnosticConsumer::Subconsumer("A", std::move(consumerA)));
239-
consumers.emplace_back(FileSpecificDiagnosticConsumer::Subconsumer(
240-
"", std::move(consumerUnaffiliated)));
229+
consumers.emplace_back("A", std::move(consumerA));
230+
consumers.emplace_back("", std::move(consumerUnaffiliated));
241231

242232
auto topConsumer =
243233
FileSpecificDiagnosticConsumer::consolidateSubconsumers(consumers);
@@ -289,10 +279,8 @@ TEST(FileSpecificDiagnosticConsumer, NotesAreAttachedToErrors) {
289279
consumerA.get(), expectedUnaffiliated);
290280

291281
SmallVector<FileSpecificDiagnosticConsumer::Subconsumer, 2> consumers;
292-
consumers.emplace_back(
293-
FileSpecificDiagnosticConsumer::Subconsumer("A", std::move(consumerA)));
294-
consumers.emplace_back(FileSpecificDiagnosticConsumer::Subconsumer(
295-
"", std::move(consumerUnaffiliated)));
282+
consumers.emplace_back("A", std::move(consumerA));
283+
consumers.emplace_back("", std::move(consumerUnaffiliated));
296284

297285
auto topConsumer =
298286
FileSpecificDiagnosticConsumer::consolidateSubconsumers(consumers);
@@ -354,10 +342,8 @@ TEST(FileSpecificDiagnosticConsumer, NotesAreAttachedToWarningsAndRemarks) {
354342
consumerA.get(), expectedUnaffiliated);
355343

356344
SmallVector<FileSpecificDiagnosticConsumer::Subconsumer, 2> consumers;
357-
consumers.emplace_back(
358-
FileSpecificDiagnosticConsumer::Subconsumer("A", std::move(consumerA)));
359-
consumers.emplace_back(FileSpecificDiagnosticConsumer::Subconsumer(
360-
"", std::move(consumerUnaffiliated)));
345+
consumers.emplace_back("A", std::move(consumerA));
346+
consumers.emplace_back("", std::move(consumerUnaffiliated));
361347

362348
auto topConsumer =
363349
FileSpecificDiagnosticConsumer::consolidateSubconsumers(consumers);
@@ -416,10 +402,8 @@ TEST(FileSpecificDiagnosticConsumer, NotesAreAttachedToErrorsEvenAcrossFiles) {
416402
consumerA.get(), expectedB);
417403

418404
SmallVector<FileSpecificDiagnosticConsumer::Subconsumer, 2> consumers;
419-
consumers.emplace_back(
420-
FileSpecificDiagnosticConsumer::Subconsumer("A", std::move(consumerA)));
421-
consumers.emplace_back(
422-
FileSpecificDiagnosticConsumer::Subconsumer("B", std::move(consumerB)));
405+
consumers.emplace_back("A", std::move(consumerA));
406+
consumers.emplace_back("B", std::move(consumerB));
423407

424408
auto topConsumer =
425409
FileSpecificDiagnosticConsumer::consolidateSubconsumers(consumers);
@@ -482,10 +466,8 @@ TEST(FileSpecificDiagnosticConsumer,
482466
consumerA.get(), expectedUnaffiliated);
483467

484468
SmallVector<FileSpecificDiagnosticConsumer::Subconsumer, 2> consumers;
485-
consumers.emplace_back(
486-
FileSpecificDiagnosticConsumer::Subconsumer("A", std::move(consumerA)));
487-
consumers.emplace_back(FileSpecificDiagnosticConsumer::Subconsumer(
488-
"", std::move(consumerUnaffiliated)));
469+
consumers.emplace_back("A", std::move(consumerA));
470+
consumers.emplace_back("", std::move(consumerUnaffiliated));
489471

490472
auto topConsumer =
491473
FileSpecificDiagnosticConsumer::consolidateSubconsumers(consumers);
@@ -540,10 +522,8 @@ TEST(FileSpecificDiagnosticConsumer,
540522
consumerA.get(), expectedUnaffiliated);
541523

542524
SmallVector<FileSpecificDiagnosticConsumer::Subconsumer, 2> consumers;
543-
consumers.emplace_back(
544-
FileSpecificDiagnosticConsumer::Subconsumer("A", std::move(consumerA)));
545-
consumers.emplace_back(FileSpecificDiagnosticConsumer::Subconsumer(
546-
"", std::move(consumerUnaffiliated)));
525+
consumers.emplace_back("A", std::move(consumerA));
526+
consumers.emplace_back("", std::move(consumerUnaffiliated));
547527

548528
auto topConsumer =
549529
FileSpecificDiagnosticConsumer::consolidateSubconsumers(consumers);

0 commit comments

Comments
 (0)