Added CheckName field to YAML report

[alexfh]

Summary

See details in https://llvm.org/bugs/show_bug.cgi?id=26132

[alexfh]

I'm not sure tooling::Replacement is the best place to store check name. Maybe create a separate wrapper class and serialize it instead (clang-apply-replacements will have to be changed to support this format as well). This could be ClangTidyDiagnostic or just Diagnostic, and we could also store the message and other useful information in it.

tools/clang/include/clang/Tooling/Core/Replacement.h

93

Please clang-format the code.

tools/clang/tools/extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

80–81

Doesn't it compile without StringRef() around Error.CheckName?

[• Elijah_Th]

• small format changes
• removed unnecessary StringRef

[alexfh]

Thanks for addressing the comments. However, the main concern is this:

In D16183#326759, @alexfh wrote:

I'm not sure tooling::Replacement is the best place to store check name. Maybe create a separate wrapper class and serialize it instead (clang-apply-replacements will have to be changed to support this format as well). This could be ClangTidyDiagnostic or just Diagnostic, and we could also store the message and other useful information in it.

[• Elijah_Th]

What kind of wrapper should it be?
I was thinking of this kind:

class ExtendedReplacement : public Replacement {
public:
  ExtendedReplacement(StringRef CheckName, Replacement &R);

  StringRef getCheckName() const { return CheckName; }
  std::string CheckName;
}

but in this case (Replacement.h:141)

typedef std::set<Replacement> Replacements;

should be changed to

typedef std::set<ExtendedReplacement> Replacements;

which means a lot of code will be changed.

Is that an acceptable change? Or you meant another kind of wrapper?

[alexfh]

In D16183#329475, @Elijah_Th wrote:

What kind of wrapper should it be?
I was thinking of this kind:

class ExtendedReplacement : public Replacement {

No, I meant a different thing: serialize ClangTidyError or make a specialized structure for this purpose. Apart from replacements it contains the diagnostic message, diagnostic location, and the check name. We'll just need to teach clang-apply-replacements to read this structure.

[• Elijah_Th]

Now the class that is serialized is Diagnostics.
I've moved ClangTidyError and ClangTidyMessage to the upper level, and renamed to Diagnostics and DiagnosticsMessage. Now any tool can use this classes, they contain more information than than Replacement. I think Diagnostics is a good class to add some more information later.

Please, review the fix, if it's ok I'll run clang-format on the changed code and provide the documentation. Thanks!

[• Elijah_Th]

Fixed YAML format (was not correct in the last patch).
Grouped replacements in YAML by Diagnostics. It will help to apply replacements for one fix at once.

[• Elijah_Th]

YAML report looks like this now:

---
MainSourceFile:  ''
Diagnostics:     
  CheckName:       misc-macro-parentheses
  Replacements:    
    - FilePath:        /media/SSD_/code/zdoom/main_1.cpp
      Offset:          1354
      Length:          0
      ReplacementText: '('
    - FilePath:        /media/SSD_/code/zdoom/main_1.cpp
      Offset:          1355
      Length:          0
      ReplacementText: ')'
Diagnostics:     
  CheckName:       misc-macro-parentheses
  Replacements:    
    - FilePath:        /media/SSD_/code/zdoom/main_1.cpp
      Offset:          1452
      Length:          0
      ReplacementText: '('
    - FilePath:        /media/SSD_/code/zdoom/main_1.cpp
      Offset:          1453
      Length:          0
      ReplacementText: ')'
...

[alexfh]

Sorry for the loooong delay. I missed you patch among all other ones. Could you rebase your patch on top of HEAD and clang-format the files you changed? I'll come back with more substantial comments early next week.

[alexfh]

The main concern here is that the clang-apply-replacements tool should be able to read this format. See the clang/tools/extra/clang-tidy/tool/run-clang-tidy.py script for an example of its usage.

[• Elijah_Th]

alexfh,

OK. I'll take a look at apply-replacements and fix if needed, in the beginning of the next week!

[alexfh]

This patch is superseded by D26137.

tools/clang/include/clang/Tooling/Core/Replacement.h

public:
 
   /// \brief Creates a replacement of the range [Offset, Offset+Length) in
   /// FilePath with ReplacementText.
   ///
   /// \param FilePath A source file accessible via a SourceManager.
   /// \param Offset The byte offset of the start of the range in the file.
   /// \param Length The length of the range in bytes.
   Replacement(StringRef FilePath, unsigned Offset, unsigned Length,
-              StringRef ReplacementText);
+              StringRef ReplacementText, StringRef CheckName = "");
 
   /// \brief Creates a Replacement of the range [Start, Start+Length) with
   /// ReplacementText.
   Replacement(const SourceManager &Sources, SourceLocation Start,
               unsigned Length, StringRef ReplacementText);
 
   /// \brief Creates a Replacement of the given range with ReplacementText.
   Replacement(const SourceManager &Sources, const CharSourceRange &Range,
               StringRef ReplacementText,
-              const LangOptions &LangOpts = LangOptions());
+              const LangOptions &LangOpts = LangOptions(),
+              StringRef CheckName = "");
 
   /// \brief Creates a Replacement of the node with ReplacementText.
   template <typename Node>
   Replacement(const SourceManager &Sources, const Node &NodeToReplace,
               StringRef ReplacementText,
               const LangOptions &LangOpts = LangOptions());
 
   /// \brief Returns whether this replacement can be applied to a file.
   ///
   /// Only replacements that are in a valid file can be applied.
   bool isApplicable() const;
 
   /// \brief Accessors.
   /// @{
   StringRef getFilePath() const { return FilePath; }
   unsigned getOffset() const { return ReplacementRange.getOffset(); }
   unsigned getLength() const { return ReplacementRange.getLength(); }
   StringRef getReplacementText() const { return ReplacementText; }
+  StringRef getCheckName() const { return CheckName; }
   /// @}
 
   /// \brief Applies the replacement on the Rewriter.
   bool apply(Rewriter &Rewrite) const;
 
   /// \brief Returns a human readable string representation.
   std::string toString() const;
 
- private:
-   void setFromSourceLocation(const SourceManager &Sources,
-                              SourceLocation Start, unsigned Length,
-                              StringRef ReplacementText);
-   void setFromSourceRange(const SourceManager &Sources,
-                           const CharSourceRange &Range,
-                           StringRef ReplacementText,
-                           const LangOptions &LangOpts);
+  void setFromSourceLocation(const SourceManager &Sources, SourceLocation Start,
+                             unsigned Length, StringRef ReplacementText,
+                             StringRef CheckName = "");
+                          const LangOptions &LangOpts,
+                          StringRef CheckName = "");
 
   std::string FilePath;
   Range ReplacementRange;
   std::string ReplacementText;
+  std::string CheckName;
 };
 
 /// \brief Less-than operator between two Replacements.
 bool operator<(const Replacement &LHS, const Replacement &RHS);
 
 /// \brief Equal-to operator between two Replacements.
 bool operator==(const Replacement &LHS, const Replacement &RHS);
 

tools/clang/include/clang/Tooling/ReplacementsYaml.h

 
 /// \brief Specialized MappingTraits to describe how a Replacement is
 /// (de)serialized.
 template <> struct MappingTraits<clang::tooling::Replacement> {
   /// \brief Helper to (de)serialize a Replacement since we don't have direct
   /// access to its data members.
   struct NormalizedReplacement {
     NormalizedReplacement(const IO &)
-        : FilePath(""), Offset(0), Length(0), ReplacementText("") {}
+        : FilePath(""), Offset(0), Length(0), ReplacementText(""),
+          CheckName("") {}
 
     NormalizedReplacement(const IO &, const clang::tooling::Replacement &R)
         : FilePath(R.getFilePath()), Offset(R.getOffset()),
-          Length(R.getLength()), ReplacementText(R.getReplacementText()) {}
+          Length(R.getLength()), ReplacementText(R.getReplacementText()),
+          CheckName(R.getCheckName()) {}
 
     clang::tooling::Replacement denormalize(const IO &) {
       return clang::tooling::Replacement(FilePath, Offset, Length,
-                                         ReplacementText);
+                                         ReplacementText, CheckName);
     }
 
     std::string FilePath;
     unsigned int Offset;
     unsigned int Length;
     std::string ReplacementText;
+    std::string CheckName;
   };
 
   static void mapping(IO &Io, clang::tooling::Replacement &R) {
     MappingNormalization<NormalizedReplacement, clang::tooling::Replacement>
         Keys(Io, R);
     Io.mapRequired("FilePath", Keys->FilePath);
     Io.mapRequired("Offset", Keys->Offset);
     Io.mapRequired("Length", Keys->Length);
     Io.mapRequired("ReplacementText", Keys->ReplacementText);
+    Io.mapRequired("CheckName", Keys->CheckName);
   }
 };
 
 /// \brief Specialized MappingTraits to describe how a
 /// TranslationUnitReplacements is (de)serialized.
 template <> struct MappingTraits<clang::tooling::TranslationUnitReplacements> {
   static void mapping(IO &Io,
                       clang::tooling::TranslationUnitReplacements &Doc) {

tools/clang/lib/Tooling/Core/Replacement.cpp

 namespace tooling {
 
 static const char * const InvalidLocation = "";
 
 Replacement::Replacement()
   : FilePath(InvalidLocation) {}
 
 Replacement::Replacement(StringRef FilePath, unsigned Offset, unsigned Length,
-                         StringRef ReplacementText)
+                         StringRef ReplacementText, StringRef CheckName)
     : FilePath(FilePath), ReplacementRange(Offset, Length),
-      ReplacementText(ReplacementText) {}
+      ReplacementText(ReplacementText), CheckName(CheckName) {}
 
 Replacement::Replacement(const SourceManager &Sources, SourceLocation Start,
                          unsigned Length, StringRef ReplacementText) {
   setFromSourceLocation(Sources, Start, Length, ReplacementText);
 }
 
 Replacement::Replacement(const SourceManager &Sources,
                          const CharSourceRange &Range,
-                         StringRef ReplacementText,
-                         const LangOptions &LangOpts) {
-  setFromSourceRange(Sources, Range, ReplacementText, LangOpts);
+                         StringRef ReplacementText, const LangOptions &LangOpts,
+                         StringRef CheckName) {
+  setFromSourceRange(Sources, Range, ReplacementText, LangOpts, CheckName);
 }
 
 bool Replacement::isApplicable() const {
   return FilePath != InvalidLocation;
 }
 
 bool Replacement::apply(Rewriter &Rewrite) const {
   SourceManager &SM = Rewrite.getSourceMgr();
bool operator==(const Replacement &LHS, const Replacement &RHS) {
   return LHS.getOffset() == RHS.getOffset() &&
          LHS.getLength() == RHS.getLength() &&
          LHS.getFilePath() == RHS.getFilePath() &&
          LHS.getReplacementText() == RHS.getReplacementText();
 }
 
 void Replacement::setFromSourceLocation(const SourceManager &Sources,
                                         SourceLocation Start, unsigned Length,
-                                        StringRef ReplacementText) {
+                                        StringRef ReplacementText,
+                                        StringRef CheckName) {
   const std::pair<FileID, unsigned> DecomposedLocation =
       Sources.getDecomposedLoc(Start);
   const FileEntry *Entry = Sources.getFileEntryForID(DecomposedLocation.first);
   this->FilePath = Entry ? Entry->getName() : InvalidLocation;
   this->ReplacementRange = Range(DecomposedLocation.second, Length);
   this->ReplacementText = ReplacementText;
+  this->CheckName = CheckName;
 }
 
 // FIXME: This should go into the Lexer, but we need to figure out how
 // to handle ranges for refactoring in general first - there is no obvious
 // good way how to integrate this into the Lexer yet.
 static int getRangeSize(const SourceManager &Sources,
                         const CharSourceRange &Range,
                         const LangOptions &LangOpts) {
   SourceLocation SpellingBegin = Sources.getSpellingLoc(Range.getBegin());
   SourceLocation SpellingEnd = Sources.getSpellingLoc(Range.getEnd());
   std::pair<FileID, unsigned> Start = Sources.getDecomposedLoc(SpellingBegin);
   std::pair<FileID, unsigned> End = Sources.getDecomposedLoc(SpellingEnd);
   if (Start.first != End.first) return -1;
   if (Range.isTokenRange())
     End.second += Lexer::MeasureTokenLength(SpellingEnd, Sources, LangOpts);
   return End.second - Start.second;
 }
 
 void Replacement::setFromSourceRange(const SourceManager &Sources,
                                      const CharSourceRange &Range,
                                      StringRef ReplacementText,
-                                     const LangOptions &LangOpts) {
+                                     const LangOptions &LangOpts,
+                                     StringRef CheckName) {
   setFromSourceLocation(Sources, Sources.getSpellingLoc(Range.getBegin()),
-                        getRangeSize(Sources, Range, LangOpts),
-                        ReplacementText);
+                        getRangeSize(Sources, Range, LangOpts), ReplacementText,
+                        CheckName);
 }
 
 template <typename T>
 unsigned shiftedCodePositionInternal(const T &Replaces, unsigned Position) {
   unsigned Offset = 0;
   for (const auto& R : Replaces) {
     if (R.getOffset() + R.getLength() <= Position) {
       Offset += R.getReplacementText().size() - R.getLength();
MergedReplacement(const Replacement &R, bool MergeSecond, int D)
         Offset(R.getOffset() + (MergeSecond ? 0 : Delta)), Length(R.getLength()),
         Text(R.getReplacementText()) {
     Delta += MergeSecond ? 0 : Text.size() - Length;
     DeltaFirst = MergeSecond ? Text.size() - Length : 0;
   }
 
   // Merges the next element 'R' into this merged element. As we always merge
   // from 'First' into 'Second' or vice versa, the MergedReplacement knows what
-  // set the next element is coming from. 
+  // set the next element is coming from.
   void merge(const Replacement &R) {
     if (MergeSecond) {
       unsigned REnd = R.getOffset() + Delta + R.getLength();
       unsigned End = Offset + Text.size();
       if (REnd > End) {
         Length += REnd - End;
         MergeSecond = false;
       }
for (auto FirstI = First.begin(), SecondI = Second.begin();
     Delta -= Merged.deltaFirst();
     Result.insert(Merged.asReplacement());
   }
   return Result;
 }
 
 } // end namespace tooling
 } // end namespace clang
-

tools/clang/tools/extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

void emitCodeContext(SourceLocation Loc, DiagnosticsEngine::Level Level,
     assert(Loc.isValid());
     for (const auto &FixIt : Hints) {
       CharSourceRange Range = FixIt.RemoveRange;
       assert(Range.getBegin().isValid() && Range.getEnd().isValid() &&
              "Invalid range in the fix-it hint.");
       assert(Range.getBegin().isFileID() && Range.getEnd().isFileID() &&
              "Only file locations supported in fix-it hints.");
 
-      Error.Fix.insert(tooling::Replacement(SM, Range, FixIt.CodeToInsert));
+      Error.Fix.insert(tooling::Replacement(SM, Range, FixIt.CodeToInsert,
+                                            LangOptions(), Error.CheckName));
     }
   }
 
   void emitIncludeLocation(SourceLocation Loc, PresumedLoc PLoc,
                            const SourceManager &SM) override {}
 
   void emitImportLocation(SourceLocation Loc, PresumedLoc PLoc,
                           StringRef ModuleName,