Remove limits on the number of fix-it hints and ranges in the DiagnosticsEngine.

Summary:
The limits on the number of fix-it hints and ranges attached to a
diagnostic are arbitrary and don't apply universally to all users of the
DiagnosticsEngine. The way the limits are enforced may lead to diagnostics
generating invalid sets of fixes. I suggest removing the limits, which will also
simplify the implementation.

Reviewers: rsmith

Reviewed By: rsmith

Subscribers: klimek, cfe-commits

Differential Revision: http://reviews.llvm.org/D3879

llvm-svn: 209468

clang/include/clang/Basic/Diagnostic.h

@@ -738,20 +738,10 @@ private:
     /// diagnostic with more than that almost certainly has to be simplified
     /// anyway.
     MaxArguments = 10,
-
-    /// \brief The maximum number of ranges we can hold.
-    MaxRanges = 10,
-
-    /// \brief The maximum number of ranges we can hold.
-    MaxFixItHints = 10
   };
 
   /// \brief The number of entries in Arguments.
   signed char NumDiagArgs;
-  /// \brief The number of ranges in the DiagRanges array.
-  unsigned char NumDiagRanges;
-  /// \brief The number of hints in the DiagFixItHints array.
-  unsigned char NumDiagFixItHints;
 
   /// \brief Specifies whether an argument is in DiagArgumentsStr or
   /// in DiagArguments.
@@ -774,11 +764,11 @@ private:
   intptr_t DiagArgumentsVal[MaxArguments];
 
   /// \brief The list of ranges added to this diagnostic.
-  CharSourceRange DiagRanges[MaxRanges];
+  SmallVector<CharSourceRange, 8> DiagRanges;
 
   /// \brief If valid, provides a hint with some code to insert, remove,
   /// or modify at a particular position.
-  FixItHint DiagFixItHints[MaxFixItHints];
+  SmallVector<FixItHint, 8> DiagFixItHints;
 
   DiagnosticMappingInfo makeMappingInfo(diag::Mapping Map, SourceLocation L) {
     bool isPragma = L.isValid();
@@ -875,7 +865,7 @@ public:
 /// for example.
 class DiagnosticBuilder {
   mutable DiagnosticsEngine *DiagObj;
-  mutable unsigned NumArgs, NumRanges, NumFixits;
+  mutable unsigned NumArgs;
 
   /// \brief Status variable indicating if this diagnostic is still active.
   ///
@@ -890,15 +880,15 @@ class DiagnosticBuilder {
 
   void operator=(const DiagnosticBuilder &) LLVM_DELETED_FUNCTION;
   friend class DiagnosticsEngine;
-  
+
   DiagnosticBuilder()
-    : DiagObj(nullptr), NumArgs(0), NumRanges(0), NumFixits(0), IsActive(false),
-      IsForceEmit(false) { }
+      : DiagObj(nullptr), NumArgs(0), IsActive(false), IsForceEmit(false) {}
 
   explicit DiagnosticBuilder(DiagnosticsEngine *diagObj)
-    : DiagObj(diagObj), NumArgs(0), NumRanges(0), NumFixits(0), IsActive(true),
-      IsForceEmit(false) {
+      : DiagObj(diagObj), NumArgs(0), IsActive(true), IsForceEmit(false) {
     assert(diagObj && "DiagnosticBuilder requires a valid DiagnosticsEngine!");
+    diagObj->DiagRanges.clear();
+    diagObj->DiagFixItHints.clear();
   }
 
   friend class PartialDiagnostic;
@@ -906,8 +896,6 @@ class DiagnosticBuilder {
 protected:
   void FlushCounts() {
     DiagObj->NumDiagArgs = NumArgs;
-    DiagObj->NumDiagRanges = NumRanges;
-    DiagObj->NumDiagFixItHints = NumFixits;
   }
 
   /// \brief Clear out the current diagnostic.
@@ -954,8 +942,6 @@ public:
     IsForceEmit = D.IsForceEmit;
     D.Clear();
     NumArgs = D.NumArgs;
-    NumRanges = D.NumRanges;
-    NumFixits = D.NumFixits;
   }
 
   /// \brief Retrieve an empty diagnostic builder.
@@ -1001,24 +987,12 @@ public:
 
   void AddSourceRange(const CharSourceRange &R) const {
     assert(isActive() && "Clients must not add to cleared diagnostic!");
-    assert(NumRanges < DiagnosticsEngine::MaxRanges &&
-           "Too many arguments to diagnostic!");
-    DiagObj->DiagRanges[NumRanges++] = R;
+    DiagObj->DiagRanges.push_back(R);
   }
 
   void AddFixItHint(const FixItHint &Hint) const {
     assert(isActive() && "Clients must not add to cleared diagnostic!");
-    assert(NumFixits < DiagnosticsEngine::MaxFixItHints &&
-           "Too many arguments to diagnostic!");
-    DiagObj->DiagFixItHints[NumFixits++] = Hint;
-  }
-
-  bool hasMaxRanges() const {
-    return NumRanges == DiagnosticsEngine::MaxRanges;
-  }
-
-  bool hasMaxFixItHints() const {
-    return NumFixits == DiagnosticsEngine::MaxFixItHints;
+    DiagObj->DiagFixItHints.push_back(Hint);
   }
 
   void addFlagValue(StringRef V) const { DiagObj->setFlagNameValue(V); }
@@ -1224,22 +1198,22 @@ public:
 
   /// \brief Return the number of source ranges associated with this diagnostic.
   unsigned getNumRanges() const {
-    return DiagObj->NumDiagRanges;
+    return DiagObj->DiagRanges.size();
   }
 
   /// \pre Idx < getNumRanges()
   const CharSourceRange &getRange(unsigned Idx) const {
-    assert(Idx < DiagObj->NumDiagRanges && "Invalid diagnostic range index!");
+    assert(Idx < getNumRanges() && "Invalid diagnostic range index!");
     return DiagObj->DiagRanges[Idx];
   }
 
   /// \brief Return an array reference for this diagnostic's ranges.
   ArrayRef<CharSourceRange> getRanges() const {
-    return llvm::makeArrayRef(DiagObj->DiagRanges, DiagObj->NumDiagRanges);
+    return DiagObj->DiagRanges;
   }
 
   unsigned getNumFixItHints() const {
-    return DiagObj->NumDiagFixItHints;
+    return DiagObj->DiagFixItHints.size();
   }
 
   const FixItHint &getFixItHint(unsigned Idx) const {
@@ -1247,8 +1221,8 @@ public:
     return DiagObj->DiagFixItHints[Idx];
   }
 
-  const FixItHint *getFixItHints() const {
-    return getNumFixItHints()? DiagObj->DiagFixItHints : nullptr;
+  ArrayRef<FixItHint> getFixItHints() const {
+    return DiagObj->DiagFixItHints;
   }
 
   /// \brief Format this diagnostic into a string, substituting the

clang/include/clang/Basic/PartialDiagnostic.h

@@ -36,7 +36,7 @@ public:
   };
 
   struct Storage {
-    Storage() : NumDiagArgs(0), NumDiagRanges(0) { }
+    Storage() : NumDiagArgs(0) { }
 
     enum {
         /// \brief The maximum number of arguments we can hold. We
@@ -50,9 +50,6 @@ public:
     /// \brief The number of entries in Arguments.
     unsigned char NumDiagArgs;
 
-    /// \brief This is the number of ranges in the DiagRanges array.
-    unsigned char NumDiagRanges;
-
     /// \brief Specifies for each argument whether it is in DiagArgumentsStr
     /// or in DiagArguments.
     unsigned char DiagArgumentsKind[MaxArguments];
@@ -69,9 +66,7 @@ public:
     std::string DiagArgumentsStr[MaxArguments];
 
     /// \brief The list of ranges added to this diagnostic.
-    ///
-    /// It currently only support 10 ranges, could easily be extended if needed.
-    CharSourceRange DiagRanges[10];
+    SmallVector<CharSourceRange, 8> DiagRanges;
 
     /// \brief If valid, provides a hint with some code to insert, remove, or
     /// modify at a particular position.
@@ -97,7 +92,6 @@ public:
 
       Storage *Result = FreeList[--NumFreeListEntries];
       Result->NumDiagArgs = 0;
-      Result->NumDiagRanges = 0;
       Result->FixItHints.clear();
       return Result;
     }
@@ -166,10 +160,7 @@ private:
     if (!DiagStorage)
       DiagStorage = getStorage();
 
-    assert(DiagStorage->NumDiagRanges <
-           llvm::array_lengthof(DiagStorage->DiagRanges) &&
-           "Too many arguments to diagnostic!");
-    DiagStorage->DiagRanges[DiagStorage->NumDiagRanges++] = R;
+    DiagStorage->DiagRanges.push_back(R);
   }
 
   void AddFixItHint(const FixItHint &Hint) const {
@@ -308,12 +299,12 @@ public:
     }
 
     // Add all ranges.
-    for (unsigned i = 0, e = DiagStorage->NumDiagRanges; i != e; ++i)
-      DB.AddSourceRange(DiagStorage->DiagRanges[i]);
+    for (const CharSourceRange &Range : DiagStorage->DiagRanges)
+      DB.AddSourceRange(Range);
 
     // Add all fix-its.
-    for (unsigned i = 0, e = DiagStorage->FixItHints.size(); i != e; ++i)
-      DB.AddFixItHint(DiagStorage->FixItHints[i]);
+    for (const FixItHint &Fix : DiagStorage->FixItHints)
+      DB.AddFixItHint(Fix);
   }
 
   void EmitToString(DiagnosticsEngine &Diags,

clang/lib/Basic/Diagnostic.cpp

@@ -323,22 +323,19 @@ void DiagnosticsEngine::Report(const StoredDiagnostic &storedDiag) {
   CurDiagID = storedDiag.getID();
   NumDiagArgs = 0;
 
-  NumDiagRanges = storedDiag.range_size();
-  assert(NumDiagRanges < DiagnosticsEngine::MaxRanges &&
-         "Too many arguments to diagnostic!");
-  unsigned i = 0;
+  DiagRanges.clear();
+  DiagRanges.reserve(storedDiag.range_size());
   for (StoredDiagnostic::range_iterator
          RI = storedDiag.range_begin(),
          RE = storedDiag.range_end(); RI != RE; ++RI)
-    DiagRanges[i++] = *RI;
+    DiagRanges.push_back(*RI);
 
-  assert(NumDiagRanges < DiagnosticsEngine::MaxFixItHints &&
-         "Too many arguments to diagnostic!");
-  NumDiagFixItHints = 0;
+  DiagFixItHints.clear();
+  DiagFixItHints.reserve(storedDiag.fixit_size());
   for (StoredDiagnostic::fixit_iterator
          FI = storedDiag.fixit_begin(),
          FE = storedDiag.fixit_end(); FI != FE; ++FI)
-    DiagFixItHints[NumDiagFixItHints++] = *FI;
+    DiagFixItHints.push_back(*FI);
 
   assert(Client && "DiagnosticConsumer not set!");
   Level DiagLevel = storedDiag.getLevel();

clang/lib/Frontend/SerializedDiagnosticPrinter.cpp

@@ -560,8 +560,7 @@ void SDiagsWriter::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
   Renderer.emitDiagnostic(Info.getLocation(), DiagLevel,
                           State->diagBuf.str(),
                           Info.getRanges(),
-                          llvm::makeArrayRef(Info.getFixItHints(),
-                                             Info.getNumFixItHints()),
+                          Info.getFixItHints(),
                           &Info.getSourceManager(),
                           &Info);
 }

clang/lib/Frontend/TextDiagnosticPrinter.cpp

@@ -154,8 +154,7 @@ void TextDiagnosticPrinter::HandleDiagnostic(DiagnosticsEngine::Level Level,
 
   TextDiag->emitDiagnostic(Info.getLocation(), Level, DiagMessageStream.str(),
                            Info.getRanges(),
-                           llvm::makeArrayRef(Info.getFixItHints(),
-                                              Info.getNumFixItHints()),
+                           Info.getFixItHints(),
                            &Info.getSourceManager());
 
   OS.flush();

clang/lib/Lex/LiteralSupport.cpp

@@ -1577,8 +1577,7 @@ bool StringLiteralParser::CopyStringFragment(const Token &Tok,
     Dummy.reserve(Fragment.size() * CharByteWidth);
     char *Ptr = Dummy.data();
 
-    while (!Builder.hasMaxRanges() &&
-           !ConvertUTF8toWide(CharByteWidth, NextFragment, Ptr, ErrorPtrTmp)) {
+    while (!ConvertUTF8toWide(CharByteWidth, NextFragment, Ptr, ErrorPtrTmp)) {
       const char *ErrorPtr = reinterpret_cast<const char *>(ErrorPtrTmp);
       NextStart = resyncUTF8(ErrorPtr, Fragment.end());
       Builder << MakeCharSourceRange(Features, SourceLoc, TokBegin,

clang/lib/Lex/PPMacroExpansion.cpp

@@ -675,13 +675,8 @@ MacroArgs *Preprocessor::ReadFunctionLikeMacroArgs(Token &MacroName,
         DiagnosticBuilder DB =
             Diag(MacroName,
                  diag::note_init_list_at_beginning_of_macro_argument);
-        for (SmallVector<SourceRange, 4>::iterator
-                 Range = InitLists.begin(), RangeEnd = InitLists.end();
-                 Range != RangeEnd; ++Range) {
-          if (DB.hasMaxRanges())
-            break;
-          DB << *Range;
-        }
+        for (const SourceRange &Range : InitLists)
+          DB << Range;
       }
       return nullptr;
     }
@@ -689,15 +684,9 @@ MacroArgs *Preprocessor::ReadFunctionLikeMacroArgs(Token &MacroName,
       return nullptr;
 
     DiagnosticBuilder DB = Diag(MacroName, diag::note_suggest_parens_for_macro);
-    for (SmallVector<SourceRange, 4>::iterator
-             ParenLocation = ParenHints.begin(), ParenEnd = ParenHints.end();
-         ParenLocation != ParenEnd; ++ParenLocation) {
-      if (DB.hasMaxFixItHints())
-        break;
-      DB << FixItHint::CreateInsertion(ParenLocation->getBegin(), "(");
-      if (DB.hasMaxFixItHints())
-        break;
-      DB << FixItHint::CreateInsertion(ParenLocation->getEnd(), ")");
+    for (const SourceRange &ParenLocation : ParenHints) {
+      DB << FixItHint::CreateInsertion(ParenLocation.getBegin(), "(");
+      DB << FixItHint::CreateInsertion(ParenLocation.getEnd(), ")");
     }
     ArgTokens.swap(FixedArgTokens);
     NumActuals = FixedNumArgs;