Add Optional overload to DiagnosticBuilder operator <<

[njames93]

clang/include/clang/Basic/Diagnostic.h

1341

Should this be disabled on functions that pass format args to the diagnostic so it will only optionally add things FixIts, Notes, Ranges...?

[lebedev.ri]

What's the use case here?

[njames93]

In D75714#1912326, @lebedev.ri wrote:

What's the use case here?

Mainly syntactic sugar, If you have a function that maybe returns a FixItHint or SourceRange you can pass it directly to the DiagnosticBuilder.

[aaron.ballman]

In D75714#1912400, @njames93 wrote:

In D75714#1912326, @lebedev.ri wrote:

What's the use case here?

Mainly syntactic sugar, If you have a function that maybe returns a FixItHint or SourceRange you can pass it directly to the DiagnosticBuilder.

This comes up with a fair amount of regularity, even just for things like printing a simple integer (consider optional attribute arguments, etc). However, it seems like we might want to express this via the format string so that we don't run into a situation where an unavailable optional argument changes the indexing in the case where it's not additional information like a source range or fixit.

[gribozavr2]

I can see it being useful for fix-its and source ranges, however I have a concern about it accidentally eating arguments that were supposed to go into a format string. For that, I second Aaron's suggestion to implement an extension for format strings. If you don't want to do that far, I think adding specific overloads for optional fix-its and optional source ranges would work, and won't trigger in unexpected situations.

[njames93]

• Removed template in favour of specific overloads

[aaron.ballman]

Is there any code we can cleanup as a result of adding these overloads? I would have expected to see some code changes justifying each additional overload, which would also give us test coverage for the changes.

[njames93]

In D75714#1913230, @aaron.ballman wrote:

Is there any code we can cleanup as a result of adding these overloads? I would have expected to see some code changes justifying each additional overload, which would also give us test coverage for the changes.

I planned to add usages in follow ups but I can add them here

[njames93]

• Add usages, removed optional on arrayrefs

[gribozavr2]

clang/include/clang/Basic/Diagnostic.h

1298

s/;// (and in functions below as well)

[njames93]

• remove unnecessary semi colons

clang-tools-extra/clang-tidy/abseil/StringFindStartswithCheck.cpp

void StringFindStartswithCheck::check(const MatchFinder::MatchResult &Result) {
 
   Diagnostic << FixItHint::CreateReplacement(
       ComparisonExpr->getSourceRange(),
       (StartswithStr + "(" + HaystackExprCode + ", " + NeedleExprCode + ")")
           .str());
 
   // Create a preprocessor #include FixIt hint (CreateIncludeInsertion checks
   // whether this already exists).
-  auto IncludeHint = IncludeInserter->CreateIncludeInsertion(
+  Diagnostic << IncludeInserter->CreateIncludeInsertion(
       Source.getFileID(ComparisonExpr->getBeginLoc()), AbseilStringsMatchHeader,
       false);
-  if (IncludeHint) {
-    Diagnostic << *IncludeHint;
-  }
 }
 
 void StringFindStartswithCheck::registerPPCallbacks(
     const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
   IncludeInserter = std::make_unique<utils::IncludeInserter>(SM, getLangOpts(),
                                                               IncludeStyle);
   PP->addPPCallbacks(IncludeInserter->CreatePPCallbacks());
 }

clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp

if (InitializationString) {
     auto Diagnostic =
         diag(MatchedDecl->getLocation(), "variable %0 is not initialized")
         << MatchedDecl
         << FixItHint::CreateInsertion(
                MatchedDecl->getLocation().getLocWithOffset(
                    MatchedDecl->getName().size()),
                InitializationString);
     if (AddMathInclude) {
-      auto IncludeHint = IncludeInserter->CreateIncludeInsertion(
+      Diagnostic << IncludeInserter->CreateIncludeInsertion(
           Source.getFileID(MatchedDecl->getBeginLoc()), MathHeader, false);
-      if (IncludeHint)
-        Diagnostic << *IncludeHint;
     }
   }
 }
 
 } // namespace cppcoreguidelines
 } // namespace tidy
 } // namespace clang

clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp

auto Diag = diag(Matched->getExprLoc(),
                      "not an integer constant expression; use gsl::at() "
                      "instead");
     if (!GslHeader.empty()) {
       Diag << FixItHint::CreateInsertion(BaseRange.getBegin(), "gsl::at(")
            << FixItHint::CreateReplacement(
                   SourceRange(BaseRange.getEnd().getLocWithOffset(1),
                               IndexRange.getBegin().getLocWithOffset(-1)),
                   ", ")
-           << FixItHint::CreateReplacement(Matched->getEndLoc(), ")");
-
-      Optional<FixItHint> Insertion = Inserter->CreateIncludeInsertion(
+           << FixItHint::CreateReplacement(Matched->getEndLoc(), ")")
+           << Inserter->CreateIncludeInsertion(
                   Result.SourceManager->getMainFileID(), GslHeader,
                   /*IsAngled=*/false);
-      if (Insertion)
-        Diag << Insertion.getValue();
     }
     return;
   }
 
   const auto *StdArrayDecl =
       Result.Nodes.getNodeAs<ClassTemplateSpecializationDecl>("type");
 
   // For static arrays, this is handled in clang-diagnostic-array-bounds.

clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp

bool MakeSmartPtrCheck::replaceNew(DiagnosticBuilder &Diag,
   }
   return true;
 }
 
 void MakeSmartPtrCheck::insertHeader(DiagnosticBuilder &Diag, FileID FD) {
   if (MakeSmartPtrFunctionHeader.empty()) {
     return;
   }
-  if (auto IncludeFixit = Inserter->CreateIncludeInsertion(
-          FD, MakeSmartPtrFunctionHeader,
-          /*IsAngled=*/MakeSmartPtrFunctionHeader == StdMemoryHeader)) {
-    Diag << *IncludeFixit;
-  }
+  Diag << Inserter->CreateIncludeInsertion(
+      /*IsAngled=*/MakeSmartPtrFunctionHeader == StdMemoryHeader);
 }
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang

clang/include/clang/Basic/Diagnostic.h

 
 #include "clang/Basic/DiagnosticIDs.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/Specifiers.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/Error.h"
 #include <cassert>
 #include <cstdint>
 #include <limits>

 
 inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
                                            ArrayRef<FixItHint> Hints) {
   for (const FixItHint &Hint : Hints)
     DB.AddFixItHint(Hint);
   return DB;
 }
 
+inline const DiagnosticBuilder &
+operator<<(const DiagnosticBuilder &DB,
+           const llvm::Optional<SourceRange> &Opt) {
+  if (Opt)
+    DB << *Opt;
+  return DB;
+}
+
+inline const DiagnosticBuilder &
+operator<<(const DiagnosticBuilder &DB,
+           const llvm::Optional<CharSourceRange> &Opt) {
+  if (Opt)
+    DB << *Opt;
+  return DB;
+}
+
+inline const DiagnosticBuilder &
+operator<<(const DiagnosticBuilder &DB, const llvm::Optional<FixItHint> &Opt) {
+  if (Opt)
+    DB << *Opt;
+  return DB;
+}
+
 /// A nullability kind paired with a bit indicating whether it used a
 /// context-sensitive keyword.
 using DiagNullabilityKind = std::pair<NullabilityKind, bool>;
 
 const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
                                     DiagNullabilityKind nullability);
 
 inline DiagnosticBuilder DiagnosticsEngine::Report(SourceLocation Loc,
inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
                                            llvm::Error &&E) {
   DB.AddString(toString(std::move(E)));
   return DB;
 }
 
 inline DiagnosticBuilder DiagnosticsEngine::Report(unsigned DiagID) {
   return Report(SourceLocation(), DiagID);
 }
 
 //===----------------------------------------------------------------------===//
 // Diagnostic
 //===----------------------------------------------------------------------===//
 
 /// A little helper class (which is basically a smart pointer that forwards
 /// info from DiagnosticsEngine) that allows clients to enquire about the
 /// currently in-flight diagnostic.
 class Diagnostic {