factoring and commenting

Author: davidungar

include/swift/AST/DiagnosticConsumer.h

@@ -136,41 +136,57 @@ class NullDiagnosticConsumer : public DiagnosticConsumer {
 /// current file.
 class FileSpecificDiagnosticConsumer : public DiagnosticConsumer {
 public:
+  class Subconsumer;
+  
+  /// Given a vector of subconsumers, return the most specific DiagnosticConsumer for that vector.
+  /// That will be a FileSpecificDiagnosticConsumer if the vector has > 1 subconsumer,
+  /// the subconsumer itself if the vector has just one, or a null pointer if there are no subconsumers.
+  /// Takes ownership of the DiagnosticConsumers specified in \p subconsumers.
+  static std::unique_ptr<DiagnosticConsumer> consolidateSubconsumers(SmallVectorImpl<Subconsumer> &subconsumers);
+    
   /// A diagnostic consumer, along with the name of the buffer that it should
   /// be associated with.
   class Subconsumer {
-  public:
-    /// The name of the buffer that a consumer should
+    friend std::unique_ptr<DiagnosticConsumer> FileSpecificDiagnosticConsumer::consolidateSubconsumers(SmallVectorImpl<Subconsumer> &subconsumers);
+    
+    /// The name of the input file that a consumer and diagnostics should
     /// be associated with. An empty string means that a consumer is not
     /// associated with any particular buffer, and should only receive
     /// diagnostics that are not in any of the other consumers' files.
-    std::string bufferName;
-
+    std::string inputFileName;
+    
+    /// The consumer (if any) for diagnostics associated with the inputFileName.
     /// A null pointer for the DiagnosticConsumer means that diagnostics for
     /// this file should not be emitted.
     std::unique_ptr<DiagnosticConsumer> consumer;
-
+    
     // Has this subconsumer ever handled a diagnostic that is an error?
-    bool handledError = false;
+    bool hasAnErrorBeenConsumed = false;
 
-    Subconsumer(std::string bufferName,
+    
+  public:
+    std::string getInputFileName() const { return inputFileName; }
+    
+    DiagnosticConsumer *getConsumer() const { return consumer.get(); }
+
+    Subconsumer(std::string inputFileName,
                 std::unique_ptr<DiagnosticConsumer> consumer)
-        : bufferName(bufferName), consumer(std::move(consumer)) {}
+        : inputFileName(inputFileName), consumer(std::move(consumer)) {}
 
     void handleDiagnostic(SourceManager &SM, SourceLoc Loc,
                           DiagnosticKind Kind,
                           StringRef FormatString,
                           ArrayRef<DiagnosticArgument> FormatArgs,
                           const DiagnosticInfo &Info) {
-      if (!consumer)
+      if (!getConsumer())
         return; // Suppress non-primary diagnostic in batch mode.
-      handledError |= Kind == DiagnosticKind::Error;
-      consumer.get()->handleDiagnostic(SM, Loc, Kind, FormatString, FormatArgs, Info);
+      hasAnErrorBeenConsumed |= Kind == DiagnosticKind::Error;
+      getConsumer()->handleDiagnostic(SM, Loc, Kind, FormatString, FormatArgs, Info);
     }
 
     void informDriverOfIncompleteBatchModeCompilation() {
-      if (!handledError && consumer)
-        consumer.get()->informDriverOfIncompleteBatchModeCompilation();
+      if (!hasAnErrorBeenConsumed && getConsumer())
+        getConsumer()->informDriverOfIncompleteBatchModeCompilation();
     }
   };
 
@@ -181,19 +197,46 @@ class FileSpecificDiagnosticConsumer : public DiagnosticConsumer {
 public:
   // The commented-out consts are there because the data does not change
   // but the swap method gets called on this structure.
-  struct ConsumerSpecificInformation {
+  class ConsumerSpecificInformation {
+  private:
+    /// The range of SourceLoc's for which diagnostics should be directed to
+    /// this subconsumer.
     /*const*/ CharSourceRange range;
-    
-    /// Index into Subconsumers vector.
-    unsigned subconsumerIndex;
 
+    /// Index into Subconsumers vector for this subconsumer.
+    /*const*/ unsigned subconsumerIndex;
+    
+  public:
     ConsumerSpecificInformation(const CharSourceRange range,
                                 unsigned subconsumerIndex)
-        : range(range), subconsumerIndex(subconsumerIndex) {}
-    
+    : range(range), subconsumerIndex(subconsumerIndex) {}
+
     Subconsumer &subconsumer(FileSpecificDiagnosticConsumer &c) const {
       return c.Subconsumers[subconsumerIndex];
     }
+    
+    /// Compare according to range:
+    bool operator < (const ConsumerSpecificInformation &right)  const {
+      auto compare = std::less<const char *>();
+      return compare(getRawLoc(      range.getEnd()).getPointer(),
+                     getRawLoc(right.range.getEnd()).getPointer());
+    }
+    
+    /// Overlaps by range:
+    bool overlaps(const ConsumerSpecificInformation &other) const {
+      return range.overlaps(other.range);
+    }
+    
+    /// Does my range end after \p loc?
+    bool endsAfter(const SourceLoc loc) const {
+      auto compare = std::less<const char *>();
+      return compare(getRawLoc(range.getEnd()).getPointer(),
+                     getRawLoc(loc).getPointer());
+    }
+    
+    bool contains(const SourceLoc loc) const {
+      return range.contains(loc);
+    }
   };
 
 private:
@@ -219,15 +262,19 @@ class FileSpecificDiagnosticConsumer : public DiagnosticConsumer {
       ConsumerSpecificInfoForSubsequentNotes = None;
 
   bool HasAnErrorBeenConsumed = false;
-
-public:
+  
+  
   /// Takes ownership of the DiagnosticConsumers specified in \p consumers.
   ///
   /// There must not be two consumers for the same file (i.e., having the same
   /// buffer name).
   explicit FileSpecificDiagnosticConsumer(
-      SmallVectorImpl<Subconsumer> &consumers);
+                                          SmallVectorImpl<Subconsumer> &consumers);
 
+public:
+  
+ 
+ 
   void handleDiagnostic(SourceManager &SM, SourceLoc Loc,
                         DiagnosticKind Kind,
                         StringRef FormatString,

lib/AST/DiagnosticConsumer.cpp

@@ -38,21 +38,32 @@ static bool hasDuplicateFileNames(
     ArrayRef<FileSpecificDiagnosticConsumer::Subconsumer> subconsumers) {
   llvm::StringSet<> seenFiles;
   for (const auto &subconsumer : subconsumers) {
-    if (subconsumer.bufferName.empty()) {
+    if (subconsumer.getInputFileName().empty()) {
       // We can handle multiple subconsumers that aren't associated with any
       // file, because they only collect diagnostics that aren't in any of the
       // special files. This isn't an important use case to support, but also
       // SmallSet doesn't handle empty strings anyway!
       continue;
     }
 
-    bool isUnique = seenFiles.insert(subconsumer.bufferName).second;
+    bool isUnique = seenFiles.insert(subconsumer.getInputFileName()).second;
     if (!isUnique)
       return true;
   }
   return false;
 }
 
+std::unique_ptr<DiagnosticConsumer> FileSpecificDiagnosticConsumer::
+consolidateSubconsumers(SmallVectorImpl<Subconsumer> &subconsumers) {
+  if (subconsumers.empty())
+    return nullptr;
+  if (subconsumers.size() == 1)
+    return std::move(subconsumers.front()).consumer;
+  // Cannot use return llvm::make_unique<FileSpecificDiagnosticConsumer>(subconsumers);
+  // because the constructor is private.
+  return std::unique_ptr<DiagnosticConsumer>(new FileSpecificDiagnosticConsumer(subconsumers));
+}
+
 FileSpecificDiagnosticConsumer::FileSpecificDiagnosticConsumer(
     SmallVectorImpl<Subconsumer> &subconsumers)
     : Subconsumers(std::move(subconsumers)) {
@@ -67,11 +78,11 @@ void FileSpecificDiagnosticConsumer::computeConsumersOrderedByRange(
   // Look up each file's source range and add it to the "map" (to be sorted).
   for (const unsigned subconsumerIndex: indices(Subconsumers)) {
     const Subconsumer &subconsumer = Subconsumers[subconsumerIndex];
-    if (subconsumer.bufferName.empty())
+    if (subconsumer.getInputFileName().empty())
       continue;
 
     Optional<unsigned> bufferID =
-        SM.getIDForBufferIdentifier(subconsumer.bufferName);
+        SM.getIDForBufferIdentifier(subconsumer.getInputFileName());
     assert(bufferID.hasValue() && "consumer registered for unknown file");
     CharSourceRange range = SM.getRangeForBuffer(bufferID.getValue());
     ConsumersOrderedByRange.emplace_back(
@@ -85,9 +96,7 @@ void FileSpecificDiagnosticConsumer::computeConsumersOrderedByRange(
   std::sort(ConsumersOrderedByRange.begin(), ConsumersOrderedByRange.end(),
             [](const ConsumerSpecificInformation &left,
                const ConsumerSpecificInformation &right) -> bool {
-              auto compare = std::less<const char *>();
-              return compare(getRawLoc(left.range.getEnd()).getPointer(),
-                             getRawLoc(right.range.getEnd()).getPointer());
+              return left < right;
             });
 
   // Check that the ranges are non-overlapping. If the files really are all
@@ -98,7 +107,7 @@ void FileSpecificDiagnosticConsumer::computeConsumersOrderedByRange(
                                 ConsumersOrderedByRange.end(),
                                 [](const ConsumerSpecificInformation &left,
                                    const ConsumerSpecificInformation &right) {
-                                  return left.range.overlaps(right.range);
+                                  return left.overlaps(right);
                                 }) &&
          "overlapping ranges despite having distinct files");
 }
@@ -122,10 +131,10 @@ FileSpecificDiagnosticConsumer::consumerSpecificInformationForLocation(
     // than trying to build a nonsensical map (and actually crashing since we
     // can't find buffers for the inputs).
     assert(!Subconsumers.empty());
-    if (!SM.getIDForBufferIdentifier(Subconsumers.begin()->bufferName)
+    if (!SM.getIDForBufferIdentifier(Subconsumers.begin()->getInputFileName())
              .hasValue()) {
       assert(llvm::none_of(Subconsumers, [&](const Subconsumer &subconsumer) {
-        return SM.getIDForBufferIdentifier(subconsumer.bufferName).hasValue();
+        return SM.getIDForBufferIdentifier(subconsumer.getInputFileName()).hasValue();
       }));
       return None;
     }
@@ -141,13 +150,11 @@ FileSpecificDiagnosticConsumer::consumerSpecificInformationForLocation(
       std::lower_bound(
           ConsumersOrderedByRange.begin(), ConsumersOrderedByRange.end(), loc,
           [](const ConsumerSpecificInformation &entry, SourceLoc loc) -> bool {
-            auto compare = std::less<const char *>();
-            return compare(getRawLoc(entry.range.getEnd()).getPointer(),
-                           getRawLoc(loc).getPointer());
+            return entry.endsAfter(loc);
           });
 
   if (possiblyContainingRangeIter != ConsumersOrderedByRange.end() &&
-      possiblyContainingRangeIter->range.contains(loc)) {
+      possiblyContainingRangeIter->contains(loc)) {
     return const_cast<ConsumerSpecificInformation *>(
         possiblyContainingRangeIter);
   }
@@ -191,7 +198,7 @@ bool FileSpecificDiagnosticConsumer::finishProcessing() {
   bool hadError = false;
   for (auto &subconsumer : Subconsumers)
     hadError |=
-        subconsumer.consumer && subconsumer.consumer->finishProcessing();
+        subconsumer.getConsumer() && subconsumer.getConsumer()->finishProcessing();
   return hadError;
 }
 

lib/FrontendTool/FrontendTool.cpp

@@ -1618,12 +1618,8 @@ createDispatchingDiagnosticConsumerIfNeeded(
           return false;
         });
   }
-
-  if (subconsumers.empty())
-    return nullptr;
-  if (subconsumers.size() == 1)
-    return std::move(subconsumers.front()).consumer;
-  return llvm::make_unique<FileSpecificDiagnosticConsumer>(subconsumers);
+  
+  return FileSpecificDiagnosticConsumer::consolidateSubconsumers(subconsumers);
 }
 
 /// Creates a diagnostic consumer that handles serializing diagnostics, based on