[clang-tidy] Zircon <fbl/limits.h> -> <limits>

[juliehockett]

Summary

Adds a check to convert <fbl/limits.h> to std <limits>.

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.

[mcgrathr]

lgtm

[Eugene.Zelenko]

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

112

Please don't use auto because 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.

190

Please highlight std with ``.

[aaron.ballman]

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

48

Does this also work on platforms where the path separator is \ instead of /? What about case insensitive file systems where it may be spelled LiMiTs.H? Does this properly handle a case like:

#define LIMITS "fbl/limits.h"
#include LIMITS

(Should add test cases for all of these scenarios.)

71

Can users specialize fbl::numeric_limits for custom integral types like they can for std::numeric_limits? If so, that should maybe be covered with a checker as well.

72

::fbl instead of fbl to cover a situation like:

namespace oops {
namespace fbl {
struct numeric_limits {};
}

void foo() {
  fbl::numeric_limits n;
}
}

[juliehockett]

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

48

Since this is a migration for our own codebase, we know we don't have any code that uses any variation other than <fbl/limits.h> and so hardcoding that is acceptable to us here.

71

Technically, they could, but there are no instances of it in the codebase.

[aaron.ballman]

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

48

Then why should this check be a public one, rather than an internal check?

[juliehockett]

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

48

I explained this on the other one, but for completeness:

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.

[aaron.ballman]

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

48

I don't think it's reasonable to expect the community to bear the maintenance burden for internal-only checks for an organization. I definitely understand the desire not to carry around a fork of clang-tidy for this, but that doesn't seem like a good reason for us to spend cycles reviewing these patches, maintaining them, and then eventually removing them.

We have some precedent in that we have clang-tidy checks for llvm coding standards or google coding standards, etc but those are also used outside of the particular community for which they're named. In this case, I don't think the functionality is useful to anyone except Google. Is that correct?

[juliehockett]

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

48

That makes sense in terms of investment from you and others (though I do appreciate the effort you've put in for reviewing it thus far), but I (and my team) are willing to bear the burden of reviewing/maintaining/removing them. If clang-tidy provided a way to tack on a set of external checks without forking, we'd happily do that, but that's not really possible right now.

Would it be reasonable to do the review/maintenance/removal of these migration checks from within our team, such that we're not asking you to spend cycles on them?

[aaron.ballman]

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

48

Would it be reasonable to do the review/maintenance/removal of these migration checks from within our team, such that we're not asking you to spend cycles on them?

Personally, I don't think so. It still impose burdens on users of clang-tidy by having checks that are not going to be useful to anyone except people from one company. These checks appears in clang-tidy list checks, contribute to binary sizes, are documented in user-facing places, users may enable them needlessly and report bugs against them, etc.

I think that we shouldn't have any checks that are only useful for (effectively) a single company -- I don't think that scales well, and I don't think it benefits the community of users. I think a better approach is to introduce plugins for clang-tidy checks. It's definitely a larger project than what you're proposing and I understand if you don't want to tackle it, but it benefits the entire community and solves the problem you want to solve.

[steveire]

This is https://bugs.llvm.org//show_bug.cgi?id=32739 . I think checks not relevant to a general audience (ie including the boost directory) should be in external plugins. It's not really clear to me what has to happen for that, but it seems increasingly relevant.

[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
+  FblLimitsCheck.cpp
   TemporaryObjectsCheck.cpp
   ZirconTidyModule.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   )

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

+//===--- FblLimitsCheck.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_FBLLIMITSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ZIRCON_FBLLIMITSCHECK_H
+
+#include "../ClangTidy.h"
+#include "../utils/IncludeInserter.h"
+
+namespace clang {
+namespace tidy {
+namespace zircon {
+
+/// Replace instances of fbl::numeric_limits with std::numeric_limits, removes
+/// <fbl/limits.h> includes and adds <limits> include if std::numeric_limits is
+/// used.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/zircon-fbl-limits.html
+class FblLimitsCheck : public ClangTidyCheck {
+public:
+  FblLimitsCheck(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_FBLLIMITSCHECK_H

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

+//===--- FblLimitsCheck.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 "FblLimitsCheck.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 {
+
+class FblLimitsPPCallbacks : public PPCallbacks {
+public:
+  explicit FblLimitsPPCallbacks(FblLimitsCheck &Check, SourceManager &SM)
+      : Check(Check), SM(SM) {}
+
+  void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok,
+                          StringRef FileName, bool IsAngled,
+                          CharSourceRange FilenameRange, const FileEntry *File,
+                          StringRef SearchPath, StringRef RelativePath,
+                          const Module *Imported,
+                          SrcMgr::CharacteristicKind FileType) override;
+  void EndOfMainFile() override;
+
+private:
+  CharSourceRange IncludeRange;
+
+  FblLimitsCheck &Check;
+  SourceManager &SM;
+};
+
+void FblLimitsPPCallbacks::InclusionDirective(
+    SourceLocation HashLoc, const Token &IncludeTok, StringRef FileName,
+    bool IsAngled, CharSourceRange FilenameRange, const FileEntry *File,
+    StringRef SearchPath, StringRef RelativePath, const Module *Imported,
+    SrcMgr::CharacteristicKind FileType) {
+  if (FileName == "fbl/limits.h") {
+    unsigned End = std::strcspn(SM.getCharacterData(HashLoc), "\n") + 1;
+    IncludeRange =
+        CharSourceRange::getCharRange(HashLoc, HashLoc.getLocWithOffset(End));
+  }
+}
+
+void FblLimitsPPCallbacks::EndOfMainFile() {
+  // Any instance of <fbl/limits.h> should be removed, since the decls in it
+  // will be replaced in this check and <limits> added.
+  if (!SM.isInMainFile(IncludeRange.getBegin())) {
+    Check.diag(IncludeRange.getBegin(),
+               "including fbl/limits.h is deprecated, transitively "
+               "included from %0")
+        << SM.getFilename(IncludeRange.getBegin())
+        << FixItHint::CreateRemoval(IncludeRange);
+    return;
+  }
+  Check.diag(IncludeRange.getBegin(), "including fbl/limits.h is deprecated")
+      << FixItHint::CreateRemoval(IncludeRange);
+}
+
+void FblLimitsCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(varDecl(hasType(cxxRecordDecl(allOf(
+                                 hasDeclContext(namespaceDecl(hasName("fbl"))),
+                                 hasName("numeric_limits")))))
+                         .bind("limits"),
+                     this);
+}
+
+void FblLimitsCheck::registerPPCallbacks(CompilerInstance &Compiler) {
+  Inserter = llvm::make_unique<utils::IncludeInserter>(
+      Compiler.getSourceManager(), Compiler.getLangOpts(), IncludeStyle);
+  Compiler.getPreprocessor().addPPCallbacks(Inserter->CreatePPCallbacks());
+  Compiler.getPreprocessor().addPPCallbacks(
+      llvm::make_unique<FblLimitsPPCallbacks>(*this,
+                                              Compiler.getSourceManager()));
+}
+
+void FblLimitsCheck::check(const MatchFinder::MatchResult &Result) {
+  SourceManager &SM = *Result.SourceManager;
+  const auto *V = Result.Nodes.getNodeAs<VarDecl>("limits");
+
+  SourceLocation Start = V->getBeginLoc();
+  SourceLocation End = V->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 LAngle = Statement.find("<");
+  if (LAngle != std::string::npos)
+    End = Start.getLocWithOffset(LAngle - 1);
+
+  DiagnosticBuilder Diag =
+      diag(Start, "use of fbl::numeric_limits is deprecated, use "
+                  "std::numeric_limits instead")
+      << FixItHint::CreateReplacement(SourceRange(Start, End),
+                                      "std::numeric_limits");
+
+  // Add in the <limits> header, since we know this file uses it.
+  if (llvm::Optional<FixItHint> IncludeFixit =
+          Inserter->CreateIncludeInsertion(SM.getFileID(Start), "limits",
+                                           /*IsAngled=*/true)) {
+    Diag << *IncludeFixit;
+  }
+}
+
+void FblLimitsCheck::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 "FblLimitsCheck.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<FblLimitsCheck>(
+        "zircon-fbl-limits");
     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-limits
+  <clang-tidy/checks/zircon-fbl-limits>` check.
+
+  Suggests converting uses of ``fbl::numeric_limits<T>`` to
+  ``std::numeric_limits<T>``, and suggests inserting the ``<limits>`` header. It
+  also suggests the removal of all uses of the ``<fbl/limits.h>`` header, as all
+  declarations therein will be replaced by the check and the appropriate
+  ``<limits>`` replacement header recommended.
+
 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-limits
    zircon-temporary-objects

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

+.. title:: clang-tidy - zircon-fbl-limits
+
+zircon-fbl-limits
+=================
+
+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::numeric_limits<T>`` to
+``std::numeric_limits<T>``, and suggests inserting the ``<limits>`` header. It
+also suggests the removal of all uses of the ``<fbl/limits.h>`` header, as all
+declarations therein will be replaced by the check and the appropriate
+``<limits>`` replacement header recommended.

clang-tools-extra/test/clang-tidy/Inputs/zircon/transitive.h

+// Transitively included headers.
+#include <fbl/limits.h>

clang-tools-extra/test/clang-tidy/zircon-fbl-limits-transitive.cpp

+// RUN: rm -rf %T/Headers
+// RUN: mkdir %T/Headers
+// RUN: cp -r %S/Inputs/zircon %T/Headers/zircon
+// RUN: %check_clang_tidy %s zircon-fbl-limits %t -- -header-filter=.* -- -std=c++11 -I %T/Headers/zircon
+// RUN: FileCheck -input-file=%T/Headers/zircon/transitive.h %s -check-prefix=CHECK-FIXES
+// RUN: rm -rf %T/Headers
+
+// transitive.h includes <fbl/limits.h>
+#include "transitive.h"
+// CHECK-NOTES: :2:1: warning: including fbl/limits.h is deprecated, transitively included from {{.*}}.
+// CHECK-FIXES-NOT: #include <fbl/limits.h>

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

+// RUN: %check_clang_tidy %s zircon-fbl-limits %t -- -- -isystem %S/Inputs/zircon
+
+#include <fbl/limits.h>
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: including fbl/limits.h is deprecated
+// CHECK-FIXES-NOT: #include <fbl/limits.h>
+// CHECK-FIXES: #include <limits>
+
+namespace fbl {
+
+template <class T>
+class numeric_limits {};
+
+#define SPECIALIZE_INT_FBL(type) \
+  template <>                    \
+  class numeric_limits<type> {};
+
+SPECIALIZE_INT_FBL(short)
+
+numeric_limits<short> lim;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use of fbl::numeric_limits is deprecated, use std::numeric_limits instead
+// CHECK-FIXES: std::numeric_limits<short> lim;
+
+} // namespace fbl
+
+namespace std {
+
+template <class T>
+class numeric_limits {};
+
+#define SPECIALIZE_INT_STD(type) \
+  template <>                    \
+  class numeric_limits<type> {};
+
+SPECIALIZE_INT_STD(short)
+
+numeric_limits<short> lim;
+
+} // namespace std
+
+#define DECLARE() fbl::numeric_limits<short> macrolim
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use of fbl::numeric_limits is deprecated, use std::numeric_limits instead
+// CHECK-FIXES: #define DECLARE() std::numeric_limits<short> macrolim
+
+int main() {
+  fbl::numeric_limits<short> fbllim;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use of fbl::numeric_limits is deprecated, use std::numeric_limits instead
+  // CHECK-FIXES: std::numeric_limits<short> fbllim;
+  std::numeric_limits<short> stdlim;
+
+  DECLARE();
+}