[clang-tidy] - Introduce abseil-prefixed-thread-annotations check.

[mbonadei]

Summary

The new clang-tidy check "abseil-prefixed-thread-annotations" checks for usages of deprecated Abseil thread annotation macros and migrates them to the new macros that are prefixed with ABSL_.

[gribozavr]

Do you have commit access?

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

43

"unprefixed" is not something that the user cares about. Try wording it the message around deprecation. Describing the fix-it in the message is also a good idea.

"annotations without the 'ABSL_' prefix are deprecated; use the new name instead"

[gribozavr]

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

43

Same suggestion for the check name. WDYT about "DeprecatedThreadAnnotationsCheck"?

[lebedev.ri]

I have the same observation as with googletest patch (D62977),
and i'd guess this applies to all these clang-tidy abseil checks (meaning the existing ones are all equally broken):
there is no checking that the new macro actually exists.

[lebedev.ri]

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

41

This isn't being tested. What happens if the macro comes from non-abseil header?

[lebedev.ri]

(Stuck reviews are bad, but blind stamping is even worse..)

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

15–16

constexpr StringLiteral ?
shouldn't this be in anonymous namespace, or be static?

37

This isn't correct as per coding style

[Eugene.Zelenko]

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

15–16

static, please, anonymous namespaces only for class declarations.

21

Please run Clang-format.

clang-tools-extra/docs/ReleaseNotes.rst

96

Please use double back-ticks for language constructs highlighting.

clang-tools-extra/docs/clang-tidy/checks/abseil-prefixed-thread-annotations.rst

6

Please use sentence from Release Notes instead.

12

Please separate with empty line.

17

Clang-format, please.

[lebedev.ri]

This review seems to be stuck/dead, consider abandoning if no longer relevant.

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

 #include "DurationDivisionCheck.h"
 #include "DurationFactoryFloatCheck.h"
 #include "DurationFactoryScaleCheck.h"
 #include "DurationSubtractionCheck.h"
 #include "DurationUnnecessaryConversionCheck.h"
 #include "FasterStrsplitDelimiterCheck.h"
 #include "NoInternalDependenciesCheck.h"
 #include "NoNamespaceCheck.h"
+#include "PrefixedThreadAnnotationsCheck.h"
 #include "RedundantStrcatCallsCheck.h"
 #include "StringFindStartswithCheck.h"
 #include "StrCatAppendCheck.h"
 #include "TimeComparisonCheck.h"
 #include "TimeSubtractionCheck.h"
 #include "UpgradeDurationConversionsCheck.h"
 
 namespace clang {
CheckFactories.registerCheck<DurationSubtractionCheck>(
         "abseil-duration-subtraction");
     CheckFactories.registerCheck<DurationUnnecessaryConversionCheck>(
         "abseil-duration-unnecessary-conversion");
     CheckFactories.registerCheck<FasterStrsplitDelimiterCheck>(
         "abseil-faster-strsplit-delimiter");
     CheckFactories.registerCheck<NoInternalDependenciesCheck>(
         "abseil-no-internal-dependencies");
     CheckFactories.registerCheck<NoNamespaceCheck>("abseil-no-namespace");
+    CheckFactories.registerCheck<PrefixedThreadAnnotationsCheck>(
+        "abseil-prefixed-thread-annotations");
     CheckFactories.registerCheck<RedundantStrcatCallsCheck>(
         "abseil-redundant-strcat-calls");
     CheckFactories.registerCheck<StrCatAppendCheck>(
         "abseil-str-cat-append");
     CheckFactories.registerCheck<StringFindStartswithCheck>(
         "abseil-string-find-startswith");
     CheckFactories.registerCheck<TimeComparisonCheck>(
         "abseil-time-comparison");

clang-tools-extra/clang-tidy/abseil/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyAbseilModule
   AbseilTidyModule.cpp
   DurationAdditionCheck.cpp
   DurationComparisonCheck.cpp
   DurationConversionCastCheck.cpp
   DurationDivisionCheck.cpp
   DurationFactoryFloatCheck.cpp
   DurationFactoryScaleCheck.cpp
   DurationRewriter.cpp
   DurationSubtractionCheck.cpp
   DurationUnnecessaryConversionCheck.cpp
   FasterStrsplitDelimiterCheck.cpp
   NoInternalDependenciesCheck.cpp
   NoNamespaceCheck.cpp
+  PrefixedThreadAnnotationsCheck.cpp
   RedundantStrcatCallsCheck.cpp
   StrCatAppendCheck.cpp
   StringFindStartswithCheck.cpp
   TimeComparisonCheck.cpp
   TimeSubtractionCheck.cpp
   UpgradeDurationConversionsCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   clangTooling
   )

clang-tools-extra/clang-tidy/abseil/PrefixedThreadAnnotationsCheck.h

+//===--- PrefixedThreadAnnotationsCheck.h - clang-tidy ----------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_PREFIXEDTHREADANNOTATIONSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_PREFIXEDTHREADANNOTATIONSCHECK_H
+
+#include "../ClangTidyCheck.h"
+#include "clang/Lex/PPCallbacks.h"
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+/// This check finds usages of deprecated Abseil thread annotation macros and
+/// migrates them to the new macros that are prefixed with `ABSL_`.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/abseil-prefixed-thread-annotations.html
+class PrefixedThreadAnnotationsCheck : public ClangTidyCheck {
+ public:
+  PrefixedThreadAnnotationsCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+
+  void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+                           Preprocessor *ModuleExpanderPP) override;
+};
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_PREFIXEDTHREADANNOTATIONSCHECK_H

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

+//===--- PrefixedThreadAnnotationsCheck.cpp - clang-tidy ------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "PrefixedThreadAnnotationsCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+constexpr char kAbseilThreadAnnotationsHeader[] =
+    "absl/base/internal/thread_annotations.h";
+
+namespace {
+
+class PrefixedThreadAnnotationsPPCallbacks : public PPCallbacks {
+ public:
+  PrefixedThreadAnnotationsPPCallbacks(ClangTidyCheck &Check,
+                                       const SourceManager &SM)
+      : Check(Check), SM(SM) {}
+
+  void MacroExpands(const Token &MacroNameTok, const MacroDefinition &MD,
+                    SourceRange Range, const MacroArgs *Args) override;
+
+ private:
+  ClangTidyCheck &Check;
+  const SourceManager &SM;
+};
+
+void PrefixedThreadAnnotationsPPCallbacks::MacroExpands(
+    const Token &MacroNameTok, const MacroDefinition &MD, SourceRange Range,
+    const MacroArgs *Args) {
+  llvm::StringRef actual_header =
+      SM.getFilename(MD.getMacroInfo()->getDefinitionLoc());
+  llvm::StringRef original_header =
+      llvm::StringRef(kAbseilThreadAnnotationsHeader);
+  if (actual_header.endswith(original_header)) {
+    FixItHint hint = FixItHint::CreateInsertion(Range.getBegin(), "ABSL_");
+    Check.diag(Range.getBegin(), "usage of unprefixed thread annotation")
+        << hint;
+  }
+}
+
+}  // End of anonymous namespace.
+
+void PrefixedThreadAnnotationsCheck::registerPPCallbacks(
+    const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
+  PP->addPPCallbacks(
+      llvm::make_unique<PrefixedThreadAnnotationsPPCallbacks>(*this, SM));
+}
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`abseil-duration-conversion-cast
   the right conversion function instead.
 
 - New :doc:`abseil-duration-unnecessary-conversion
   <clang-tidy/checks/abseil-duration-unnecessary-conversion>` check.
 
   Finds and fixes cases where ``absl::Duration`` values are being converted to
   numeric types and back again.
 
+- New :doc:`abseil-prefixed-thread-annotations
+  <clang-tidy/checks/abseil-prefixed-thread-annotations>` check.
+
+  Checks for usages of deprecated Abseil thread annotation macros and
+  migrates them to the new macros that are prefixed with `ABSL_`.
+
 - New :doc:`abseil-time-comparison
   <clang-tidy/checks/abseil-time-comparison>` check.
 
   Prefer comparisons in the ``absl::Time`` domain instead of the integer
   domain.
 
 - New :doc:`abseil-time-subtraction
   <clang-tidy/checks/abseil-time-subtraction>` check.

clang-tools-extra/docs/clang-tidy/checks/abseil-prefixed-thread-annotations.rst

+.. title:: clang-tidy - abseil-prefixed-thread-annotations
+
+abseil-prefixed-thread-annotations
+==================================
+
+This check finds usages of deprecated Abseil thread annotation macros and
+migrates them to the new macros that are prefixed with `ABSL_`.
+
+Examples:
+
+.. code-block:: c++
+// Original - Usage of deprecated and unprefixed Abseil thread annotations.
+#include "absl/base/thread_annotations.h"
+
+class X {
+  // ...
+  private:
+    absl::Mutex m_;
+    int state_ GUARDED_BY(m_);
+};
+
+// Suggestion - Addition of ABSL_ prefix.
+class X {
+  // ...
+  private:
+    absl::Mutex m_;
+    int state_ ABSL_GUARDED_BY(m_);
+};

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. toctree::
    abseil-duration-division
    abseil-duration-factory-float
    abseil-duration-factory-scale
    abseil-duration-subtraction
    abseil-duration-unnecessary-conversion
    abseil-faster-strsplit-delimiter
    abseil-no-internal-dependencies
    abseil-no-namespace
+   abseil-prefixed-thread-annotations
    abseil-redundant-strcat-calls
    abseil-str-cat-append
    abseil-string-find-startswith
    abseil-time-comparison
    abseil-time-subtraction
    abseil-upgrade-duration-conversions
    android-cloexec-accept
    android-cloexec-accept4

clang-tools-extra/test/clang-tidy/Inputs/absl/base/internal/thread_annotations.h

+#define SCOPED_LOCKABLE

clang-tools-extra/test/clang-tidy/Inputs/absl/base/thread_annotations.h

+#include "absl/base/internal/thread_annotations.h"
+
+#define ABSL_SCOPED_LOCKABLE

clang-tools-extra/test/clang-tidy/abseil-prefixed-thread-annotations.cpp

+// RUN: %check_clang_tidy %s abseil-prefixed-thread-annotations %t -- -- -I%S/Inputs
+
+#include "absl/base/thread_annotations.h"
+
+class SCOPED_LOCKABLE X {};
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: usage of unprefixed thread annotation [abseil-prefixed-thread-annotations]
+// CHECK-FIXES: class ABSL_SCOPED_LOCKABLE X {};
+
+class ABSL_SCOPED_LOCKABLE Y {};

llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/abseil/BUILD.gn

sources = [
     "DurationFactoryFloatCheck.cpp",
     "DurationFactoryScaleCheck.cpp",
     "DurationRewriter.cpp",
     "DurationSubtractionCheck.cpp",
     "DurationUnnecessaryConversionCheck.cpp",
     "FasterStrsplitDelimiterCheck.cpp",
     "NoInternalDependenciesCheck.cpp",
     "NoNamespaceCheck.cpp",
+    "PrefixedThreadAnnotationsCheck.cpp",
     "RedundantStrcatCallsCheck.cpp",
     "StrCatAppendCheck.cpp",
     "StringFindStartswithCheck.cpp",
     "TimeComparisonCheck.cpp",
     "TimeSubtractionCheck.cpp",
     "UpgradeDurationConversionsCheck.cpp",
   ]
 }