[clang-tidy] Remark was added to clang tooling Diagnostic

[ivanmurashko]

Summary

The diff adds Remark to Diagnostic::Level for clang tooling. That makes Remark diagnostic level ready to use in clang-tidy checks: the clang-diagnostic-module-import becomes visible as a part of the change

Test plan:

ninja check-clang
ninja check-clang-tools

[DmitryPolukhin]

clang/include/clang/Tooling/Core/Diagnostic.h

72

nit, I would move remark first to have list by increasing severity. It looks like this enum values is not used outside of the sources so I hope it is safe.

[aaron.ballman]

LGTM aside from the nit from @DmitryPolukhin

clang/include/clang/Tooling/Core/Diagnostic.h

72

We have diagnostic notes as well and it's not clear to me how to order remark and note should we go that way in the future. So I'm not opposed to the suggestion, but the order likely doesn't matter all that much.

[ivanmurashko]

@DmitryPolukhin suggestion was applied

[aaron.ballman]

Thanks! Do you need someone to commit on your behalf? If so, what name and email address would you like used for patch attribution?

[ivanmurashko]

Hi @aaron.ballman

Thanks! Do you need someone to commit on your behalf?

I will appreciate your assistance in the diff landing.

If so, what name and email address would you like used for patch attribution?

It would be good to use mine - Ivan Murashko (ivan.murashko at gmail.com) for that

Note: I noticed some tests failed after rebase but they seem to be unrelated to my change and also failed on main branch

clang/include/clang/Tooling/Core/Diagnostic.h

72

@DmitryPolukhin fixed

[aaron.ballman]

In D102906#2777296, @ivanmurashko wrote:

Hi @aaron.ballman

Thanks! Do you need someone to commit on your behalf?

I will appreciate your assistance in the diff landing.

Thank you for the patch! I've landed on your behalf in 7f2f0247f855b143c12cd33d1f53d14bc63f3e82.

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

if (CheckName.empty()) {
       switch (DiagLevel) {
       case DiagnosticsEngine::Error:
       case DiagnosticsEngine::Fatal:
         CheckName = "clang-diagnostic-error";
         break;
       case DiagnosticsEngine::Warning:
         CheckName = "clang-diagnostic-warning";
         break;
+      case DiagnosticsEngine::Remark:
+        CheckName = "clang-diagnostic-remark";
+        break;
       default:
         CheckName = "clang-diagnostic-unknown";
         break;
       }
     }
 
     ClangTidyError::Level Level = ClangTidyError::Warning;
     if (DiagLevel == DiagnosticsEngine::Error ||
         DiagLevel == DiagnosticsEngine::Fatal) {
       // Force reporting of Clang errors regardless of filters and non-user
       // code.
       Level = ClangTidyError::Error;
       LastErrorRelatesToUserCode = true;
       LastErrorPassesLineFilter = true;
+    } else if (DiagLevel == DiagnosticsEngine::Remark) {
+      Level = ClangTidyError::Remark;
     }
+
     bool IsWarningAsError = DiagLevel == DiagnosticsEngine::Warning &&
                             Context.treatAsError(CheckName);
     Errors.emplace_back(CheckName, Level, Context.getCurrentBuildDirectory(),
                         IsWarningAsError);
   }
 
   if (ExternalDiagEngine) {
     // If there is an external diagnostics engine, like in the

clang-tools-extra/test/clang-tidy/infrastructure/Inputs/remarks/A.h

+// A

clang-tools-extra/test/clang-tidy/infrastructure/Inputs/remarks/module.modulemap

+module A { header "A.h" }

clang-tools-extra/test/clang-tidy/infrastructure/remarks.cpp

+// RUN: rm -rf %t
+// RUN: cp -r %S/Inputs/remarks %t
+// RUN: cp %s %t/t.cpp
+
+// RUN: clang-tidy -checks='-*,modernize-use-override,clang-diagnostic-module-import' t.cpp -- \
+// RUN:     -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache \
+// RUN:     -fsyntax-only \
+// RUN:     -I%S/Inputs/remarks \
+// RUN:     -working-directory=%t \
+// RUN:     -Rmodule-build -Rmodule-import t.cpp 2>&1 |\
+// RUN: FileCheck %s -implicit-check-not "remark:"
+
+#include "A.h"
+// CHECK: remark: importing module 'A' from {{.*}} [clang-diagnostic-module-import]
+

clang/include/clang/Tooling/Core/Diagnostic.h

struct DiagnosticMessage {
   llvm::SmallVector<FileByteRange, 1> Ranges;
 };
 
 /// Represents the diagnostic with the level of severity and possible
 /// fixes to be applied.
 struct Diagnostic {
   enum Level {
     Warning = DiagnosticsEngine::Warning,
-    Error = DiagnosticsEngine::Error
+    Error = DiagnosticsEngine::Error,
+    Remark = DiagnosticsEngine::Remark
   };
 
   Diagnostic() = default;
 
   Diagnostic(llvm::StringRef DiagnosticName, Level DiagLevel,
              StringRef BuildDirectory);
 
   Diagnostic(llvm::StringRef DiagnosticName, const DiagnosticMessage &Message,

clang/include/clang/Tooling/DiagnosticsYaml.h

static void mapping(IO &Io, clang::tooling::TranslationUnitDiagnostics &Doc) {
     Io.mapRequired("Diagnostics", Doc.Diagnostics);
   }
 };
 
 template <> struct ScalarEnumerationTraits<clang::tooling::Diagnostic::Level> {
   static void enumeration(IO &IO, clang::tooling::Diagnostic::Level &Value) {
     IO.enumCase(Value, "Warning", clang::tooling::Diagnostic::Warning);
     IO.enumCase(Value, "Error", clang::tooling::Diagnostic::Error);
+    IO.enumCase(Value, "Remark", clang::tooling::Diagnostic::Remark);
   }
 };
 
 } // end namespace yaml
 } // end namespace llvm
 
 #endif // LLVM_CLANG_TOOLING_DIAGNOSTICSYAML_H

clang/unittests/Tooling/DiagnosticsYamlTest.cpp

static FileByteRange makeByteRange(int FileOffset,
   Range.FilePath = FilePath;
   return Range;
 }
 
 static Diagnostic makeDiagnostic(StringRef DiagnosticName,
                                  const std::string &Message, int FileOffset,
                                  const std::string &FilePath,
                                  const StringMap<Replacements> &Fix,
-                                 const SmallVector<FileByteRange, 1> &Ranges) {
+                                 const SmallVector<FileByteRange, 1> &Ranges,
+                                 Diagnostic::Level DiagnosticLevel) {
   return Diagnostic(DiagnosticName,
                     makeMessage(Message, FileOffset, FilePath, Fix, Ranges), {},
-                    Diagnostic::Warning, "path/to/build/directory");
+                    DiagnosticLevel, "path/to/build/directory");
 }
 
 static const char *YAMLContent =
     "---\n"
     "MainSourceFile:  'path/to/source.cpp'\n"
     "Diagnostics:\n"
     "  - DiagnosticName:  'diagnostic#1\'\n"
     "    DiagnosticMessage:\n"
static const char *YAMLContent =
     "        FileOffset:      88\n"
     "        Replacements:    []\n"
     "      - Message:         Note2\n"
     "        FilePath:        'path/to/note2.cpp'\n"
     "        FileOffset:      99\n"
     "        Replacements:    []\n"
     "    Level:           Warning\n"
     "    BuildDirectory:  'path/to/build/directory'\n"
+    "  - DiagnosticName:  'diagnostic#4'\n"
+    "    DiagnosticMessage:\n"
+    "      Message:         'message #4'\n"
+    "      FilePath:        'path/to/source3.cpp'\n"
+    "      FileOffset:      72\n"
+    "      Replacements:    []\n"
+    "    Level:           Remark\n"
+    "    BuildDirectory:  'path/to/build/directory'\n"
     "...\n";
 
 TEST(DiagnosticsYamlTest, serializesDiagnostics) {
   TranslationUnitDiagnostics TUD;
   TUD.MainSourceFile = "path/to/source.cpp";
 
   StringMap<Replacements> Fix1 = {
       {"path/to/source.cpp",
        Replacements({"path/to/source.cpp", 100, 12, "replacement #1"})}};
   TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#1", "message #1", 55,
-                                           "path/to/source.cpp", Fix1, {}));
+                                           "path/to/source.cpp", Fix1, {},
+                                           Diagnostic::Warning));
 
   StringMap<Replacements> Fix2 = {
       {"path/to/header.h",
        Replacements({"path/to/header.h", 62, 2, "replacement #2"})}};
   SmallVector<FileByteRange, 1> Ranges2 =
       {makeByteRange(10, 10, "path/to/source.cpp")};
   TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#2", "message #2", 60,
-                                           "path/to/header.h", Fix2, Ranges2));
+                                           "path/to/header.h", Fix2, Ranges2,
+                                           Diagnostic::Warning));
 
   TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#3", "message #3", 72,
-                                           "path/to/source2.cpp", {}, {}));
+                                           "path/to/source2.cpp", {}, {},
+                                           Diagnostic::Warning));
   TUD.Diagnostics.back().Notes.push_back(
       makeMessage("Note1", 88, "path/to/note1.cpp", {}, {}));
   TUD.Diagnostics.back().Notes.push_back(
       makeMessage("Note2", 99, "path/to/note2.cpp", {}, {}));
 
+  TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#4", "message #4", 72,
+                                           "path/to/source3.cpp", {}, {},
+                                           Diagnostic::Remark));
+
   std::string YamlContent;
   raw_string_ostream YamlContentStream(YamlContent);
 
   yaml::Output YAML(YamlContentStream);
   YAML << TUD;
 
   EXPECT_EQ(YAMLContent, YamlContentStream.str());
 }
 
 TEST(DiagnosticsYamlTest, deserializesDiagnostics) {
   TranslationUnitDiagnostics TUDActual;
   yaml::Input YAML(YAMLContent);
   YAML >> TUDActual;
 
   ASSERT_FALSE(YAML.error());
-  ASSERT_EQ(3u, TUDActual.Diagnostics.size());
+  ASSERT_EQ(4u, TUDActual.Diagnostics.size());
   EXPECT_EQ("path/to/source.cpp", TUDActual.MainSourceFile);
 
   auto getFixes = [](const StringMap<Replacements> &Fix) {
     std::vector<Replacement> Fixes;
     for (auto &Replacements : Fix) {
       for (auto &Replacement : Replacements.second) {
         Fixes.push_back(Replacement);
       }