[clang-tidy] Zircon fbl::move -> std::move

[juliehockett]

Summary

Adds a check to convert fbl::move to std::move.

This check is part of a set of migration checks as we prepare to move Zircon user code to use the C++ standard library, and should prevent regressions after the migration is complete.

[JonasToth]

I think this check is ok in the current form, but does it really need to be in upstream clang-tidy? You said its for migration only, so it is not valuable for you for a long time either?

clang-tools-extra/clang-tidy/zircon/FblMoveCheck.h

21

Please highlight code with quotes (single or backticks)

clang-tools-extra/test/clang-tidy/zircon-fbl-move.cpp

6

please add test for aliases, function pointers and similar indirections.

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/zircon/FblMoveCheck.cpp

62

Please don't use auto when type could not be deduced from statement.

clang-tools-extra/docs/ReleaseNotes.rst

188

I think first statement is not necessary. Second should start from Suggests. Will be good idea to synchronize it with documentation.

[juliehockett]

In D54168#1289128, @JonasToth wrote:

I think this check is ok in the current form, but does it really need to be in upstream clang-tidy? You said its for migration only, so it is not valuable for you for a long time either?

So yes, this check is for a migration, and we would delete it once regressions weren't possible. We would like the suite to be in upstream, however, because we use the ToT llvm/clang/tools/etc, and don't want to have to fork just to use clang-tidy for this sort of thing. Since clang-tidy doesn't provide any way to have external checks to the tool itself, upstreaming is the most ideal option.

Orthogonal to our particular build setup, it'd also be nice to have an example of this sort of migration done by clang-tidy in-tree. There has been a lot of discussion recently about doing migrations with clang-tidy, but it's always describing an internal migration that uses a forked tree and a private suite of checks that can't be released.

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

188

Please try to fill as much as possible to 80 characters.

189

inserting -> to include? Same in documentation.

[juliehockett]

After a lot of discussion, we'll do this migration internally. Thanks for your comments!

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

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyZirconModule
+  FblMoveCheck.cpp
   TemporaryObjectsCheck.cpp
   ZirconTidyModule.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   )

clang-tools-extra/clang-tidy/zircon/FblMoveCheck.h

+//===--- FblMoveCheck.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_ZIRCON_FBLINCLUDENEWCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ZIRCON_FBLINCLUDENEWCHECK_H
+
+#include "../ClangTidy.h"
+#include "../utils/IncludeInserter.h"
+
+namespace clang {
+namespace tidy {
+namespace zircon {
+
+/// Replace instances of `fbl::move` with `std::move`, and adds the
+/// `<utility>` header if a replacement occurs.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/zircon-fbl-move.html
+class FblMoveCheck : public ClangTidyCheck {
+public:
+  FblMoveCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context),
+        IncludeStyle(utils::IncludeSorter::parseIncludeStyle(
+            Options.getLocalOrGlobal("IncludeStyle", "google"))) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void registerPPCallbacks(clang::CompilerInstance &Compiler) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+
+private:
+  std::unique_ptr<utils::IncludeInserter> Inserter;
+  const utils::IncludeSorter::IncludeStyle IncludeStyle;
+};
+
+} // namespace zircon
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ZIRCON_FBLINCLUDENEWCHECK_H

clang-tools-extra/clang-tidy/zircon/FblMoveCheck.cpp

+//===--- FblMoveCheck.cpp - clang-tidy ------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "FblMoveCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Lex/Lexer.h"
+#include "llvm/ADT/Optional.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace zircon {
+
+void FblMoveCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      callExpr(callee(functionDecl(
+                   allOf(hasDeclContext(namespaceDecl(hasName("::fbl"))),
+                         hasName("move")))))
+          .bind("move"),
+      this);
+  Finder->addMatcher(
+      declRefExpr(hasDeclaration(functionDecl(
+                      allOf(hasDeclContext(namespaceDecl(hasName("::fbl"))),
+                            hasName("move")))))
+          .bind("move"),
+      this);
+}
+
+void FblMoveCheck::registerPPCallbacks(CompilerInstance &Compiler) {
+  Inserter = llvm::make_unique<utils::IncludeInserter>(
+      Compiler.getSourceManager(), Compiler.getLangOpts(), IncludeStyle);
+  Compiler.getPreprocessor().addPPCallbacks(Inserter->CreatePPCallbacks());
+}
+
+void FblMoveCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *E = Result.Nodes.getNodeAs<Expr>("move");
+
+  // Find the end of the name of the call.
+  SourceManager &SM = *Result.SourceManager;
+  SourceLocation Start = E->getBeginLoc();
+  SourceLocation End = E->getEndLoc();
+
+  // If it's a macro, we want to extract information about the macro instead of
+  // the expr.
+  if (Start.isMacroID()) {
+    Start = SM.getSpellingLoc(Start);
+    End = SM.getSpellingLoc(End);
+  }
+
+  bool Invalid = false;
+  StringRef Statement = Lexer::getSourceText(
+      CharSourceRange::getCharRange(Start, End), SM, getLangOpts(), &Invalid);
+  if (Invalid)
+    return;
+
+  size_t LParen = Statement.find('(');
+  if (LParen == 0)
+    // There isn't a function, so something's gone wrong.
+    return;
+  if (LParen == std::string::npos)
+    // If there isn't an lparen, we want the whole statement.
+    End = Start.getLocWithOffset(Statement.size());
+  else
+    // Remove the lparen from the size range, since we just want the call.
+    End = Start.getLocWithOffset(LParen - 1);
+
+  DiagnosticBuilder Diag =
+      diag(Start, "fbl::move is deprecated, use std::move instead")
+      << FixItHint::CreateReplacement(SourceRange(Start, End), "std::move");
+
+  // Add in the <utility> header.
+  if (llvm::Optional<FixItHint> IncludeFixit =
+          Inserter->CreateIncludeInsertion(SM.getFileID(Start), "utility",
+                                           /*IsAngled=*/true)) {
+    Diag << *IncludeFixit;
+  }
+}
+
+void FblMoveCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "IncludeStyle", IncludeStyle);
+}
+
+} // namespace zircon
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/zircon/ZirconTidyModule.cpp

 //===--- ZirconTidyModule.cpp - clang-tidy---------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "FblMoveCheck.h"
 #include "TemporaryObjectsCheck.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace zircon {
 
 /// This module is for Zircon-specific checks.
 class ZirconModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<FblMoveCheck>(
+        "zircon-fbl-move");
     CheckFactories.registerCheck<TemporaryObjectsCheck>(
         "zircon-temporary-objects");
   }
 };
 
 // Register the ZirconTidyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<ZirconModule>
     X("zircon-module", "Adds Zircon kernel checks.");
 } // namespace zircon
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the ZirconModule.
 volatile int ZirconModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

- New alias :doc:`hicpp-uppercase-literal-suffix
   :doc:`readability-uppercase-literal-suffix
   <clang-tidy/checks/readability-uppercase-literal-suffix>`
   added.
 
 - The :doc:`readability-redundant-smartptr-get
   <clang-tidy/checks/readability-redundant-smartptr-get>` check does not warn
   about calls inside macros anymore by default.
 
+- New :doc:`zircon-fbl-move
+  <clang-tidy/checks/zircon-fbl-include-new>` check.
+
+  Suggests converting uses of ``fbl::move`` to ``std::move``, and suggests
+  to include the ``<utility>`` header.
+
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to modularize
 --------------------------
 
 The improvements are...

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

.. toctree::
    readability-redundant-string-init
    readability-simplify-boolean-expr
    readability-simplify-subscript-expr
    readability-static-accessed-through-instance
    readability-static-definition-in-anonymous-namespace
    readability-string-compare
    readability-uniqueptr-delete-release
    readability-uppercase-literal-suffix
+   zircon-fbl-move
    zircon-temporary-objects

clang-tools-extra/docs/clang-tidy/checks/zircon-fbl-move.rst

+.. title:: clang-tidy - zircon-fbl-move
+
+zircon-fbl-move
+===============
+
+This check is part of the migration checks for moving Zircon user code to use
+the C++ standard library.
+
+It suggests converting uses of ``fbl::move`` to ``std::move``, and suggests
+to include the ``<utility>`` header.
+

clang-tools-extra/test/clang-tidy/zircon-fbl-move.cpp

+// RUN: %check_clang_tidy %s zircon-fbl-move %t
+
+// CHECK-FIXES: #include <utility>
+
+namespace fbl {
+
+void move(int i) {}
+
+} // namespace fbl
+
+namespace std {
+
+void move(int i) {}
+
+} // namespace std
+
+#define MOVE(i) fbl::move(i)
+// CHECK-NOTES: [[@LINE-1]]:17: warning: fbl::move is deprecated, use std::move instead
+// CHECK-FIXES: #define MOVE(i) std::move(i)
+
+int main() {
+  int i = 0;
+  fbl::move(i);
+  // CHECK-NOTES: [[@LINE-1]]:3: warning: fbl::move is deprecated, use std::move instead
+  // CHECK-FIXES: std::move(i)
+
+  int j = 0;
+  std::move(j);
+
+  void (*foo)(int);
+  foo = &fbl::move;
+  // CHECK-NOTES: [[@LINE-1]]:10: warning: fbl::move is deprecated, use std::move instead
+  // CHECK-FIXES: foo = &std::move
+
+  int k = 0;
+  MOVE(k);
+}