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 ↗

(On Diff #44862)

Please clang-format the code.

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

80 ↗

(On Diff #44862)

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.

/media/SSD_/code/LLVM-code/LLVM-trunk_256412/llvm/tools/clang/include/clang/Tooling/Core/Diagnostics.h

+//===--- Replacement.h - Framework for clang refactoring tools --*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+//  Classes supporting refactorings that span multiple translation units.
+//  While single translation unit refactorings are supported via the Rewriter,
+//  when refactoring multiple translation units changes must be stored in a
+//  SourceManager independent form, duplicate changes need to be removed, and
+//  all changes must be applied at once at the end of the refactoring so that
+//  the code is always parseable.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLING_CORE_DIAGNOSTICS_H
+#define LLVM_CLANG_TOOLING_CORE_DIAGNOSTICS_H
+
+#include "clang/Basic/Diagnostic.h"
+#include "llvm/ADT/SmallVector.h"
+#include "Replacement.h"
+#include <string>
+
+namespace clang {
+    namespace tooling {
+
+        struct DiagnosticsMessage {
+            DiagnosticsMessage(StringRef Message = "");
+            DiagnosticsMessage(StringRef Message, const SourceManager &Sources,
+                    SourceLocation Loc);
+            std::string Message;
+            std::string FilePath;
+            unsigned FileOffset;
+        };
+
+        struct Diagnostics {
+
+            enum Level {
+                Ignored = DiagnosticsEngine::Ignored,
+                Warning = DiagnosticsEngine::Warning,
+                Error = DiagnosticsEngine::Error
+            };
+            
+            Diagnostics();
+
+            Diagnostics(StringRef CheckName, Level DiagLevel);
+
+            std::string CheckName;
+            DiagnosticsMessage Message;
+            tooling::Replacements Fix;
+            SmallVector<DiagnosticsMessage, 1> Notes;
+
+            Level DiagLevel;
+        };
+        
+        struct TranslationUnitDiagnostics {
+          std::string MainSourceFile;
+
+          std::string Context;
+
+          std::vector<Diagnostics> Diags;
+};
+
+    }
+}
+
+#endif

/media/SSD_/code/LLVM-code/LLVM-trunk_256412/llvm/tools/clang/include/clang/Tooling/DiagnosticsYaml.h

+//===-- ReplacementsYaml.h -- Serialiazation for Replacements ---*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+///
+/// \file
+/// \brief This file defines the structure of a YAML document for serializing
+/// ClangTidy errors.
+///
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLING_DIAGNOSTICSYAML_H
+#define LLVM_CLANG_TOOLING_DIAGNOSTICSYAML_H
+
+#include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Core/Diagnostics.h"
+#include "llvm/Support/YAMLTraits.h"
+#include <string>
+#include <vector>
+
+LLVM_YAML_IS_SEQUENCE_VECTOR(clang::tooling::Diagnostics)
+
+namespace llvm {
+namespace yaml {
+    
+template <> struct MappingTraits<clang::tooling::Diagnostics> {
+  static void mapping(IO &Io,
+                      clang::tooling::Diagnostics &D) {
+    std::vector<clang::tooling::Replacement> fixes(D.Fix.begin(), D.Fix.end());
+    Io.mapRequired("CheckName", D.CheckName);
+    Io.mapRequired("Replacements", fixes);
+  }
+};
+
+template <> struct MappingTraits<clang::tooling::TranslationUnitDiagnostics> {
+  static void mapping(IO &Io,
+                      clang::tooling::TranslationUnitDiagnostics &TUD) {
+    Io.mapRequired("MainSourceFile", TUD.MainSourceFile);
+    Io.mapOptional("Context", TUD.Context, std::string());
+    for (clang::tooling::Diagnostics diag : TUD.Diags) {
+        if (diag.Fix.size() > 0) {
+            Io.mapRequired("Diagnostics", diag);
+        }
+    }
+  }
+};
+} // end namespace yaml
+} // end namespace llvm
+
+#endif /* LLVM_CLANG_TOOLING_DIAGNOSTICSYAML_H */
+

/media/SSD_/code/LLVM-code/LLVM-trunk_256412/llvm/tools/clang/lib/Tooling/Core/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangToolingCore
   Lookup.cpp
   Replacement.cpp
+  Diagnostics.cpp
 
   LINK_LIBS
   clangAST
   clangBasic
   clangLex
   clangRewrite
   )

/media/SSD_/code/LLVM-code/LLVM-trunk_256412/llvm/tools/clang/lib/Tooling/Core/Diagnostics.cpp

+//===--- Replacement.cpp - Framework for clang refactoring tools ----------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+//  Implements classes to support/store refactorings.
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/Tooling/Core/Diagnostics.h"
+#include "clang/Basic/SourceManager.h"
+
+namespace clang {
+    namespace tooling {
+
+        DiagnosticsMessage::DiagnosticsMessage(StringRef Message)
+        : Message(Message), FileOffset(0) {
+        }
+
+        DiagnosticsMessage::DiagnosticsMessage(StringRef Message,
+                const SourceManager &Sources,
+                SourceLocation Loc)
+        : Message(Message) {
+            assert(Loc.isValid() && Loc.isFileID());
+            FilePath = Sources.getFilename(Loc);
+            FileOffset = Sources.getFileOffset(Loc);
+        }
+        
+        Diagnostics::Diagnostics() : DiagLevel(Ignored) {}
+
+        Diagnostics::Diagnostics(StringRef CheckName,
+                Diagnostics::Level DiagLevel)
+        : CheckName(CheckName), DiagLevel(DiagLevel) {
+        }   
+    }
+}

/media/SSD_/code/LLVM-code/LLVM-trunk_256412/llvm/tools/clang/tools/extra/clang-tidy/ClangTidy.cpp

 #include "clang/Frontend/TextDiagnosticPrinter.h"
 #include "clang/Lex/PPCallbacks.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Rewrite/Frontend/FixItRewriter.h"
 #include "clang/Rewrite/Frontend/FrontendActions.h"
 #include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
 #include "clang/Tooling/Refactoring.h"
 #include "clang/Tooling/ReplacementsYaml.h"
+#include "clang/Tooling/DiagnosticsYaml.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/Signals.h"
 #include <algorithm>
 #include <utility>
 
 using namespace clang::ast_matchers;
 using namespace clang::driver;
void handleErrors(const std::vector<ClangTidyError> &Errors, bool Fix) {
   ErrorReporter Reporter(Fix);
   for (const ClangTidyError &Error : Errors)
     Reporter.reportDiagnostic(Error);
   Reporter.Finish();
 }
 
 void exportReplacements(const std::vector<ClangTidyError> &Errors,
                         raw_ostream &OS) {
-  tooling::TranslationUnitReplacements TUR;
-  for (const ClangTidyError &Error : Errors)
-    TUR.Replacements.insert(TUR.Replacements.end(), Error.Fix.begin(),
-                            Error.Fix.end());
-
   yaml::Output YAML(OS);
-  YAML << TUR;
+  TranslationUnitDiagnostics TUD;
+  TUD.Diags.insert(TUD.Diags.end(), Errors.begin(), Errors.end());
+  YAML << TUD;
 }
 
 } // namespace tidy
 } // namespace clang

/media/SSD_/code/LLVM-code/LLVM-trunk_256412/llvm/tools/clang/tools/extra/clang-tidy/ClangTidyDiagnosticConsumer.h

 //===--- ClangTidyDiagnosticConsumer.h - clang-tidy -------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGTIDYDIAGNOSTICCONSUMER_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGTIDYDIAGNOSTICCONSUMER_H
 
 #include "ClangTidyOptions.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Core/Diagnostics.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/Regex.h"
 #include "llvm/Support/Timer.h"
 
 namespace clang {
 
 class ASTContext;
 class CompilerInstance;
 namespace ast_matchers {
 class MatchFinder;
 }
 namespace tooling {
 class CompilationDatabase;
 }
 
 namespace tidy {
 
-/// \brief A message from a clang-tidy check.
-///
-/// Note that this is independent of a \c SourceManager.
-struct ClangTidyMessage {
-  ClangTidyMessage(StringRef Message = "");
-  ClangTidyMessage(StringRef Message, const SourceManager &Sources,
-                   SourceLocation Loc);
-  std::string Message;
-  std::string FilePath;
-  unsigned FileOffset;
-};
-
-/// \brief A detected error complete with information to display diagnostic and
-/// automatic fix.
-///
-/// This is used as an intermediate format to transport Diagnostics without a
-/// dependency on a SourceManager.
-///
-/// FIXME: Make Diagnostics flexible enough to support this directly.
-struct ClangTidyError {
-  enum Level {
-    Warning = DiagnosticsEngine::Warning,
-    Error = DiagnosticsEngine::Error
-  };
-
-  ClangTidyError(StringRef CheckName, Level DiagLevel);
-
-  std::string CheckName;
-  ClangTidyMessage Message;
-  tooling::Replacements Fix;
-  SmallVector<ClangTidyMessage, 1> Notes;
-
-  Level DiagLevel;
-};
+typedef clang::tooling::DiagnosticsMessage ClangTidyMessage;
+typedef clang::tooling::Diagnostics ClangTidyError;
 
 /// \brief Read-only set of strings represented as a list of positive and
 /// negative globs. Positive globs add all matched strings to the set, negative
 /// globs remove them in the order of appearance in the list.
 class GlobList {
 public:
   /// \brief \p GlobList is a comma-separated list of globs (only '*'
   /// metacharacter is supported) with optional '-' prefix to denote exclusion.

/media/SSD_/code/LLVM-code/LLVM-trunk_256412/llvm/tools/clang/tools/extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

void endDiagnostic(DiagOrStoredDiag D,
     assert(!Error.Message.Message.empty() && "Message has not been set");
   }
 
 private:
   ClangTidyError &Error;
 };
 } // end anonymous namespace
 
-ClangTidyMessage::ClangTidyMessage(StringRef Message)
-    : Message(Message), FileOffset(0) {}
-
-ClangTidyMessage::ClangTidyMessage(StringRef Message,
-                                   const SourceManager &Sources,
-                                   SourceLocation Loc)
-    : Message(Message) {
-  assert(Loc.isValid() && Loc.isFileID());
-  FilePath = Sources.getFilename(Loc);
-  FileOffset = Sources.getFileOffset(Loc);
-}
-
-ClangTidyError::ClangTidyError(StringRef CheckName,
-                               ClangTidyError::Level DiagLevel)
-    : CheckName(CheckName), DiagLevel(DiagLevel) {}
-
 // Returns true if GlobList starts with the negative indicator ('-'), removes it
 // from the GlobList.
 static bool ConsumeNegativeIndicator(StringRef &GlobList) {
   if (GlobList.startswith("-")) {
     GlobList = GlobList.substr(1);
     return true;
   }
   return false;