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

[alexfh]

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.

[alexfh]

Removed unrelated files.

[rsmith]

Looks great. I wonder why it wasn't always done this way...

lib/Format/Format.cpp

485 ↗

(On Diff #9714)

Unrelated change?

[alexfh]

Removed unrelated file.

[alexfh]

lib/Format/Format.cpp

485 ↗

(On Diff #9714)

Yes, thanks for noticing!

include/clang/Basic/Diagnostic.h

private:
 
   enum {
     /// \brief The maximum number of arguments we can hold.
     ///
     /// We currently only support up to 10 arguments (%0-%9).  A single
     /// 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.
   ///
   /// This is an array of ArgumentKind::ArgumentKind enum values, one for each
   /// argument.
   unsigned char DiagArgumentsKind[MaxArguments];
 
   /// \brief Holds the values of each string argument for the current
   /// diagnostic.
   ///
   /// This is only used when the corresponding ArgumentKind is ak_std_string.
   std::string DiagArgumentsStr[MaxArguments];
 
   /// \brief The values for the various substitution positions.
   ///
   /// This is used when the argument is not an std::string.  The specific
   /// value is mangled into an intptr_t and the interpretation depends on
   /// exactly what sort of argument kind it is.
   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();
     DiagnosticMappingInfo MappingInfo = DiagnosticMappingInfo::Make(
       Map, /*IsUser=*/true, isPragma);
 
     // If this is a pragma mapping, then set the diagnostic mapping flags so
     // that we override command line options.

 ///
 /// Note that many of these will be created as temporary objects (many call
 /// sites), so we want them to be small and we never want their address taken.
 /// This ensures that compilers with somewhat reasonable optimizers will promote
 /// the common fields to registers, eliminating increments of the NumArgs field,
 /// 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.
   ///
   // NOTE: This field is redundant with DiagObj (IsActive iff (DiagObj == 0)),
   // but LLVM is not currently smart enough to eliminate the null check that
   // Emit() would end up with if we used that as our status variable.
   mutable bool IsActive;
 
   /// \brief Flag indicating that this diagnostic is being emitted via a
   /// call to ForceEmit.
   mutable bool IsForceEmit;
 
   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;
   
 protected:
   void FlushCounts() {
     DiagObj->NumDiagArgs = NumArgs;
-    DiagObj->NumDiagRanges = NumRanges;
-    DiagObj->NumDiagFixItHints = NumFixits;
   }
 
   /// \brief Clear out the current diagnostic.
   void Clear() const {
     DiagObj = nullptr;
     IsActive = false;
     IsForceEmit = false;
   }
public:
   /// Copy constructor.  When copied, this "takes" the diagnostic info from the
   /// input and neuters it.
   DiagnosticBuilder(const DiagnosticBuilder &D) {
     DiagObj = D.DiagObj;
     IsActive = D.IsActive;
     IsForceEmit = D.IsForceEmit;
     D.Clear();
     NumArgs = D.NumArgs;
-    NumRanges = D.NumRanges;
-    NumFixits = D.NumFixits;
   }
 
   /// \brief Retrieve an empty diagnostic builder.
   static DiagnosticBuilder getEmpty() {
     return DiagnosticBuilder();
   }
 
   /// \brief Emits the diagnostic.
void AddTaggedVal(intptr_t V, DiagnosticsEngine::ArgumentKind Kind) const {
     assert(NumArgs < DiagnosticsEngine::MaxArguments &&
            "Too many arguments to diagnostic!");
     DiagObj->DiagArgumentsKind[NumArgs] = Kind;
     DiagObj->DiagArgumentsVal[NumArgs++] = V;
   }
 
   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); }
 };
 
 struct AddFlagValue {
   explicit AddFlagValue(StringRef V) : Val(V) {}
   StringRef Val;
public:
   intptr_t getRawArg(unsigned Idx) const {
     assert(getArgKind(Idx) != DiagnosticsEngine::ak_std_string &&
            "invalid argument accessor!");
     return DiagObj->DiagArgumentsVal[Idx];
   }
 
   /// \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 {
     assert(Idx < getNumFixItHints() && "Invalid index!");
     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
   /// formal arguments into the %0 slots.
   ///
   /// The result is appended onto the \p OutStr array.
   void FormatDiagnostic(SmallVectorImpl<char> &OutStr) const;
 

include/clang/Basic/PartialDiagnostic.h

enum {
       // The MaxArguments and MaxFixItHints member enum values from
       // DiagnosticsEngine are private but DiagnosticsEngine declares
       // PartialDiagnostic a friend.  These enum values are redeclared
       // here so that the nested Storage class below can access them.
       MaxArguments = DiagnosticsEngine::MaxArguments
   };
 
   struct Storage {
-    Storage() : NumDiagArgs(0), NumDiagRanges(0) { }
+    Storage() : NumDiagArgs(0) { }
 
     enum {
         /// \brief The maximum number of arguments we can hold. We
         /// currently only support up to 10 arguments (%0-%9).
         ///
         /// A single diagnostic with more than that almost certainly has to
         /// be simplified anyway.
         MaxArguments = PartialDiagnostic::MaxArguments
     };
 
     /// \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];
 
     /// \brief The values for the various substitution positions.
     ///
     /// This is used when the argument is not an std::string. The specific value
     /// is mangled into an intptr_t and the interpretation depends on exactly
     /// what sort of argument kind it is.
     intptr_t DiagArgumentsVal[MaxArguments];
 
     /// \brief The values for the various substitution positions that have
     /// string arguments.
     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.
     SmallVector<FixItHint, 6>  FixItHints;
   };
 
   /// \brief An allocator for Storage objects, which uses a small cache to
   /// objects, used to reduce malloc()/free() traffic for partial diagnostics.
public:
 
     /// \brief Allocate new storage.
     Storage *Allocate() {
       if (NumFreeListEntries == 0)
         return new Storage;
 
       Storage *Result = FreeList[--NumFreeListEntries];
       Result->NumDiagArgs = 0;
-      Result->NumDiagRanges = 0;
       Result->FixItHints.clear();
       return Result;
     }
 
     /// \brief Free the given storage object.
     void Deallocate(Storage *S) {
       if (S >= Cached && S <= Cached + NumCached) {
         FreeList[NumFreeListEntries++] = S;
else if (Allocator != reinterpret_cast<StorageAllocator *>(~uintptr_t(0)))
       delete DiagStorage;
     DiagStorage = nullptr;
   }
 
   void AddSourceRange(const CharSourceRange &R) const {
     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 {
     if (Hint.isNull())
       return;
 
     if (!DiagStorage)
       DiagStorage = getStorage();
for (unsigned i = 0, e = DiagStorage->NumDiagArgs; i != e; ++i) {
             == DiagnosticsEngine::ak_std_string)
         DB.AddString(DiagStorage->DiagArgumentsStr[i]);
       else
         DB.AddTaggedVal(DiagStorage->DiagArgumentsVal[i],
             (DiagnosticsEngine::ArgumentKind)DiagStorage->DiagArgumentsKind[i]);
     }
 
     // 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,
                     SmallVectorImpl<char> &Buf) const {
     // FIXME: It should be possible to render a diagnostic to a string without
     //        messing with the state of the diagnostics engine.
     DiagnosticBuilder DB(Diags.Report(getDiagID()));
     Emit(DB);

lib/Basic/Diagnostic.cpp

 
 void DiagnosticsEngine::Report(const StoredDiagnostic &storedDiag) {
   assert(CurDiagID == ~0U && "Multiple diagnostics in flight at once!");
 
   CurDiagLoc = storedDiag.getLocation();
   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();
   Diagnostic Info(this, storedDiag.getMessage());
   Client->HandleDiagnostic(DiagLevel, Info);
   if (Client->IncludeInDiagnosticCounts()) {
     if (DiagLevel == DiagnosticsEngine::Warning)
       ++NumWarnings;

lib/Frontend/SerializedDiagnosticPrinter.cpp

void SDiagsWriter::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
   }
 
   assert(Info.hasSourceManager() && LangOpts &&
          "Unexpected diagnostic with valid location outside of a source file");
   SDiagsRenderer Renderer(*this, *LangOpts, &*State->DiagOpts);
   Renderer.emitDiagnostic(Info.getLocation(), DiagLevel,
                           State->diagBuf.str(),
                           Info.getRanges(),
-                          llvm::makeArrayRef(Info.getFixItHints(),
-                                             Info.getNumFixItHints()),
+                          Info.getFixItHints(),
                           &Info.getSourceManager(),
                           &Info);
 }
 
 static serialized_diags::Level getStableLevel(DiagnosticsEngine::Level Level) {
   switch (Level) {
 #define CASE(X) case DiagnosticsEngine::X: return serialized_diags::X;
   CASE(Ignored)

lib/Frontend/TextDiagnosticPrinter.cpp

void TextDiagnosticPrinter::HandleDiagnostic(DiagnosticsEngine::Level Level,
   // Assert that the rest of our infrastructure is setup properly.
   assert(DiagOpts && "Unexpected diagnostic without options set");
   assert(Info.hasSourceManager() &&
          "Unexpected diagnostic with no source manager");
   assert(TextDiag && "Unexpected diagnostic outside source file processing");
 
   TextDiag->emitDiagnostic(Info.getLocation(), Level, DiagMessageStream.str(),
                            Info.getRanges(),
-                           llvm::makeArrayRef(Info.getFixItHints(),
-                                              Info.getNumFixItHints()),
+                           Info.getFixItHints(),
                            &Info.getSourceManager());
 
   OS.flush();
 }

lib/Lex/LiteralSupport.cpp

if (Diags) {
     const char *NextStart = resyncUTF8(ErrorPtr, Fragment.end());
     StringRef NextFragment(NextStart, Fragment.end()-NextStart);
 
     // Decode into a dummy buffer.
     SmallString<512> Dummy;
     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,
                                      ErrorPtr, NextStart);
       NextFragment = StringRef(NextStart, Fragment.end()-NextStart);
     }
   }
   return !NoErrorOnBadEncoding;

lib/Lex/PPMacroExpansion.cpp

if (!isVariadic && NumActuals > MinArgsExpected &&
     unsigned FixedNumArgs = 0;
     SmallVector<SourceRange, 4> ParenHints, InitLists;
     if (!GenerateNewArgTokens(*this, ArgTokens, FixedArgTokens, FixedNumArgs,
                               ParenHints, InitLists)) {
       if (!InitLists.empty()) {
         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;
     }
     if (FixedNumArgs != MinArgsExpected)
       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;
   }
 
   // See MacroArgs instance var for description of this.
   bool isVarargsElided = false;