From 720823a55c4d59550a17e27e4b1b81fc4eeebbcf Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Fri, 24 May 2019 11:06:09 +0100 Subject: [PATCH 01/28] Don't use CMake option command for non-booleans --- CMakeLists.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index bddbe88bf006c..0f702c5d80290 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -17,8 +17,8 @@ option(USE_HOME_DIR "Use user's home directory for save files." "ON" ) option(LOCALIZE "Support for language localizations. Also enable UTF support." "ON" ) option(LANGUAGES "Compile localization files for specified languages." "" ) option(DYNAMIC_LINKING "Use dynamic linking. Or use static to remove MinGW dependency instead." "ON") -option(GIT_BINARY "Git binary name or path." "") -OPTION(PREFIX "Location of Data & GFX directories" "") +set(GIT_BINARY "" CACHE STRING "Git binary name or path.") +set(PREFIX "" CACHE STRING "Location of Data & GFX directories") include(CTest) From b0a8a5095e0e5627cc556744d5294007c99dd611 Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Fri, 24 May 2019 22:16:29 +0100 Subject: [PATCH 02/28] Initial version of a no-long check This requires a version of clang-tidy that supports plugins, which the regular clang-tidy does not. --- CMakeLists.txt | 5 +++++ tools/clang-tidy-plugin/CMakeLists.txt | 14 ++++++++++++ tools/clang-tidy-plugin/CataTidyModule.cpp | 24 ++++++++++++++++++++ tools/clang-tidy-plugin/NoLongCheck.cpp | 26 ++++++++++++++++++++++ tools/clang-tidy-plugin/NoLongCheck.h | 22 ++++++++++++++++++ 5 files changed, 91 insertions(+) create mode 100644 tools/clang-tidy-plugin/CMakeLists.txt create mode 100644 tools/clang-tidy-plugin/CataTidyModule.cpp create mode 100644 tools/clang-tidy-plugin/NoLongCheck.cpp create mode 100644 tools/clang-tidy-plugin/NoLongCheck.h diff --git a/CMakeLists.txt b/CMakeLists.txt index 0f702c5d80290..66a503d0c49c7 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -17,6 +17,8 @@ option(USE_HOME_DIR "Use user's home directory for save files." "ON" ) option(LOCALIZE "Support for language localizations. Also enable UTF support." "ON" ) option(LANGUAGES "Compile localization files for specified languages." "" ) option(DYNAMIC_LINKING "Use dynamic linking. Or use static to remove MinGW dependency instead." "ON") +option(CATA_CLANG_TIDY_PLUGIN "Build Cata's custom clang-tidy plugin" "OFF") +set(LLVM_INCLUDE_DIRS "" CACHE STRING "Paths to llvm and clang headers used for clang-tidy plugin") set(GIT_BINARY "" CACHE STRING "Git binary name or path.") set(PREFIX "" CACHE STRING "Location of Data & GFX directories") @@ -341,6 +343,9 @@ if (NOT MSVC) add_subdirectory(src/chkjson) endif() add_subdirectory(tests) +if (CATA_CLANG_TIDY_PLUGIN) + add_subdirectory(tools/clang-tidy-plugin) +endif() CONFIGURE_FILE( "${CMAKE_CURRENT_SOURCE_DIR}/cmake_uninstall.cmake.in" diff --git a/tools/clang-tidy-plugin/CMakeLists.txt b/tools/clang-tidy-plugin/CMakeLists.txt new file mode 100644 index 0000000000000..a808de62016b1 --- /dev/null +++ b/tools/clang-tidy-plugin/CMakeLists.txt @@ -0,0 +1,14 @@ +add_library( + CataAnalyzerPlugin MODULE + CataTidyModule.cpp NoLongCheck.cpp) +MESSAGE("Building clang-tidy plugin with include paths: ${LLVM_INCLUDE_DIRS}") +if ("${LLVM_INCLUDE_DIRS}" STREQUAL "") +else() + target_include_directories( + CataAnalyzerPlugin SYSTEM PRIVATE ${LLVM_INCLUDE_DIRS}) +endif() +if(MSVC) +else() + target_compile_options( + CataAnalyzerPlugin PRIVATE -fno-exceptions -fno-rtti) +endif() diff --git a/tools/clang-tidy-plugin/CataTidyModule.cpp b/tools/clang-tidy-plugin/CataTidyModule.cpp new file mode 100644 index 0000000000000..16248a6554616 --- /dev/null +++ b/tools/clang-tidy-plugin/CataTidyModule.cpp @@ -0,0 +1,24 @@ +#include "ClangTidy.h" +#include "ClangTidyModule.h" +#include "ClangTidyModuleRegistry.h" +#include "NoLongCheck.h" + +namespace clang { +namespace tidy { +namespace cata { + +class CataModule : public ClangTidyModule { +public: + void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { + CheckFactories.registerCheck( "cata-no-long" ); + } +}; + +} // namespace cata + +// Register the MiscTidyModule using this statically initialized variable. +static ClangTidyModuleRegistry::Add + X("cata-module", "Adds Cataclysm-DDA checks."); + +} // namespace tidy +} // namespace clang diff --git a/tools/clang-tidy-plugin/NoLongCheck.cpp b/tools/clang-tidy-plugin/NoLongCheck.cpp new file mode 100644 index 0000000000000..864a6dc1e2d94 --- /dev/null +++ b/tools/clang-tidy-plugin/NoLongCheck.cpp @@ -0,0 +1,26 @@ +#include "NoLongCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace cata { + +void NoLongCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher(valueDecl(hasType(asString("long"))).bind("decl"), this); +} + +void NoLongCheck::check(const MatchFinder::MatchResult &Result) { + const auto *MatchedDecl = Result.Nodes.getNodeAs("decl"); + if( !MatchedDecl || !MatchedDecl->getLocation().isValid() ) { + return; + } + diag(MatchedDecl->getLocation(), "Variable %0 declared as long. Prefer int or int64_t.") + << MatchedDecl; +} + +} // namespace cata +} // namespace tidy +} // namespace clang diff --git a/tools/clang-tidy-plugin/NoLongCheck.h b/tools/clang-tidy-plugin/NoLongCheck.h new file mode 100644 index 0000000000000..caa388b7b5deb --- /dev/null +++ b/tools/clang-tidy-plugin/NoLongCheck.h @@ -0,0 +1,22 @@ +#ifndef CATA_TOOLS_CLANG_TIDY_NOLONGCHECK_H +#define CATA_TOOLS_CLANG_TIDY_NOLONGCHECK_H + +#include "ClangTidy.h" + +namespace clang { +namespace tidy { +namespace cata { + +class NoLongCheck : public ClangTidyCheck { +public: + NoLongCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace cata +} // namespace tidy +} // namespace clang + +#endif // CATA_TOOLS_CLANG_TIDY_NOLONGCHECK_H From 53d02331e099a8748f9737f8b97c89222a24e53c Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Fri, 24 May 2019 22:17:37 +0100 Subject: [PATCH 03/28] Enable cata checks in clang-tidy --- .clang-tidy | 1 + 1 file changed, 1 insertion(+) diff --git a/.clang-tidy b/.clang-tidy index 8fa05aa4dae96..4c42ea906b772 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -6,6 +6,7 @@ # fix their errors or recategorise them as checks we don't care about. Checks: "\ bugprone-*,\ +cata-*,\ cert-*,\ -cert-err58-cpp,\ clang-diagnostic-*,\ From 3b73b6f086f9654552ab7cda4fa79126ac79985a Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Sun, 26 May 2019 22:31:06 +0100 Subject: [PATCH 04/28] Try to support building and running on Travis This was previously working for my local LLVM build. Now I want it to work on Travis, so I have prepared a release of clang-tidy that's suitable to run there, and set it up to work with either version. --- CMakeLists.txt | 2 ++ tools/clang-tidy-plugin/CMakeLists.txt | 43 ++++++++++++++++++++++++-- 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 66a503d0c49c7..ee10e65d452be 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -8,6 +8,8 @@ SET(CMAKE_MODULE_PATH ${CMAKE_SOURCE_DIR}/CMakeModules ) +SET(CMAKE_TLS_VERIFY ON) + # Build options option(TILES "Build graphical tileset version." "OFF") option(CURSES "Build curses version." "ON" ) diff --git a/tools/clang-tidy-plugin/CMakeLists.txt b/tools/clang-tidy-plugin/CMakeLists.txt index a808de62016b1..ab576164d8777 100644 --- a/tools/clang-tidy-plugin/CMakeLists.txt +++ b/tools/clang-tidy-plugin/CMakeLists.txt @@ -1,12 +1,49 @@ +include(ExternalProject) + +find_package(LLVM REQUIRED CONFIG) +find_package(Clang REQUIRED CONFIG) + add_library( CataAnalyzerPlugin MODULE CataTidyModule.cpp NoLongCheck.cpp) -MESSAGE("Building clang-tidy plugin with include paths: ${LLVM_INCLUDE_DIRS}") -if ("${LLVM_INCLUDE_DIRS}" STREQUAL "") + +target_include_directories( + CataAnalyzerPlugin SYSTEM PRIVATE + ${LLVM_INCLUDE_DIRS} ${CLANG_INCLUDE_DIRS}) + +if ("${CATA_CLANG_TIDY_INCLUDE_DIR}" STREQUAL "") + SET(ctps_releases + https://github.com/jbytheway/clang-tidy-plugin-support/releases/download) + SET(ctps_version llvm-8.0.1-r12) + SET(ctps_src + ${CMAKE_CURRENT_BINARY_DIR}/clang-tidy-plugin-support) + + ExternalProject_Add( + clang-tidy-plugin-support + URL ${ctps_releases}/${ctps_version}/clang-tidy-plugin-support-${ctps_version}.tar.xz + URL_HASH SHA256=00ffab0df11250f394830735514c62ae787bd2eb6eb9d5e97471206d270c54e2 + SOURCE_DIR ${ctps_src} + CONFIGURE_COMMAND "" + BUILD_COMMAND "" + INSTALL_COMMAND "" + TEST_COMMAND "" + ) + + add_dependencies(CataAnalyzerPlugin clang-tidy-plugin-support) + target_include_directories( + CataAnalyzerPlugin SYSTEM PRIVATE ${ctps_src}/include) else() target_include_directories( - CataAnalyzerPlugin SYSTEM PRIVATE ${LLVM_INCLUDE_DIRS}) + CataAnalyzerPlugin SYSTEM PRIVATE ${CATA_CLANG_TIDY_INCLUDE_DIR}) endif() + +target_compile_definitions( + CataAnalyzerPlugin PRIVATE ${LLVM_DEFINITIONS}) + +# We need to turn off exceptions and RTTI to match the LLVM build. +# I feel there ought to be a way to extract these flags from the +# LLVMConfig.cmake as we have done for e.g. LLVM_INCLUDE_DIRS above, but I +# haven't found one. if(MSVC) else() target_compile_options( From 8f5cb007f34de64ac6acc30cd0f06a6288a52196 Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Sun, 26 May 2019 22:51:26 +0100 Subject: [PATCH 05/28] build.sh: Replace a 3 with $num_jobs --- build-scripts/build.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build-scripts/build.sh b/build-scripts/build.sh index cc8da37a77a55..bccb407192ddf 100755 --- a/build-scripts/build.sh +++ b/build-scripts/build.sh @@ -95,7 +95,7 @@ then analyze_files_in_random_order "$remaining_cpp_files" else # Regular build - make -j3 + make -j$num_jobs cd .. # Run regular tests [ -f "${bin_path}cata_test" ] && run_tests "${bin_path}cata_test" From 91946b25182b06a4f2253593a30f4016675d2bd6 Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Sun, 26 May 2019 22:53:39 +0100 Subject: [PATCH 06/28] Configure Travis to run clang-tidy plugin --- .travis.yml | 4 ++-- build-scripts/build.sh | 18 ++++++++++++++++++ build-scripts/clang-tidy-wrapper.sh | 9 ++++++++- 3 files changed, 28 insertions(+), 3 deletions(-) diff --git a/.travis.yml b/.travis.yml index 5e0562eaff1c6..daa7ee0419d64 100644 --- a/.travis.yml +++ b/.travis.yml @@ -105,12 +105,12 @@ jobs: osx_image: xcode10.1 compiler: clang - - env: CLANG=clang++-8 TILES=1 SOUND=1 CXXFLAGS=-Wno-unused-command-line-argument CMAKE=1 CATA_CLANG_TIDY=clang-tidy-8 + - env: CLANG=clang++-8 TILES=1 SOUND=1 CXXFLAGS=-Wno-unused-command-line-argument CMAKE=1 CATA_CLANG_TIDY=plugin name: "Clang-tidy CMake build with Tiles and Sound" compiler: clang addons: &clang8 apt: - packages: ["clang-8", "clang-tidy-8", "libsdl2-dev", "libsdl2-ttf-dev", "libsdl2-image-dev", "libsdl2-mixer-dev"] + packages: ["clang-8", "libclang-8-dev", "llvm-8-dev", "libsdl2-dev", "libsdl2-ttf-dev", "libsdl2-image-dev", "libsdl2-mixer-dev"] sources: [*apt_sources, llvm-toolchain-xenial-8] # Finally check the compiler variants diff --git a/build-scripts/build.sh b/build-scripts/build.sh index bccb407192ddf..90ecca84bd905 100755 --- a/build-scripts/build.sh +++ b/build-scripts/build.sh @@ -41,6 +41,17 @@ then build_type=Debug fi + cmake_extra_opts= + + if [ "$CATA_CLANG_TIDY" = "plugin" ] + then + cmake_extra_opts="$cmake_extra_opts -DCATA_CLANG_TIDY_PLUGIN=ON" + # Need to specify the particular LLVM / Clang versions to use, lest it + # use the llvm-7 that comes by default on the Travis Xenial image. + cmake_extra_opts="$cmake_extra_opts -DLLVM_DIR=/usr/lib/llvm-8/lib/cmake/llvm" + cmake_extra_opts="$cmake_extra_opts -DClang_DIR=/usr/lib/llvm-8/lib/cmake/clang" + fi + mkdir build cd build cmake \ @@ -49,9 +60,16 @@ then -DCMAKE_BUILD_TYPE="$build_type" \ -DTILES=${TILES:-0} \ -DSOUND=${SOUND:-0} \ + $cmake_extra_opts \ .. if [ -n "$CATA_CLANG_TIDY" ] then + if [ "$CATA_CLANG_TIDY" = "plugin" ] + then + make -j$num_jobs CataAnalyzerPlugin + CATA_CLANG_TIDY=$PWD/tools/clang-tidy-plugin/clang-tidy-plugin-support/bin/clang-tidy + fi + "$CATA_CLANG_TIDY" --version # Run clang-tidy analysis instead of regular build & test diff --git a/build-scripts/clang-tidy-wrapper.sh b/build-scripts/clang-tidy-wrapper.sh index 187248f004bb0..8ca192cedae43 100755 --- a/build-scripts/clang-tidy-wrapper.sh +++ b/build-scripts/clang-tidy-wrapper.sh @@ -15,5 +15,12 @@ then exit 0 fi +plugin=build/tools/clang-tidy-plugin/libCataAnalyzerPlugin.so + set -x -"$CATA_CLANG_TIDY" "$@" +if [ -f "$plugin" ] +then + LD_PRELOAD=$plugin "$CATA_CLANG_TIDY" "$@" +else + "$CATA_CLANG_TIDY" "$@" +fi From 5c72b741a5846d49fd24c3e1c064e72f5bc923f2 Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Mon, 27 May 2019 15:06:15 +0100 Subject: [PATCH 07/28] Add tests for NoLongCheck --- .travis.yml | 2 +- build-scripts/build.sh | 11 +++++++++- build-scripts/requirements.sh | 2 +- tools/clang-tidy-plugin/CMakeLists.txt | 3 +++ tools/clang-tidy-plugin/test/.clang-tidy | 1 + tools/clang-tidy-plugin/test/lit.cfg | 23 ++++++++++++++++++++ tools/clang-tidy-plugin/test/lit.site.cfg.in | 6 +++++ tools/clang-tidy-plugin/test/no-long.cpp | 4 ++++ 8 files changed, 49 insertions(+), 3 deletions(-) create mode 100644 tools/clang-tidy-plugin/test/.clang-tidy create mode 100644 tools/clang-tidy-plugin/test/lit.cfg create mode 100644 tools/clang-tidy-plugin/test/lit.site.cfg.in create mode 100644 tools/clang-tidy-plugin/test/no-long.cpp diff --git a/.travis.yml b/.travis.yml index daa7ee0419d64..1aaca516a5538 100644 --- a/.travis.yml +++ b/.travis.yml @@ -110,7 +110,7 @@ jobs: compiler: clang addons: &clang8 apt: - packages: ["clang-8", "libclang-8-dev", "llvm-8-dev", "libsdl2-dev", "libsdl2-ttf-dev", "libsdl2-image-dev", "libsdl2-mixer-dev"] + packages: ["clang-8", "libclang-8-dev", "llvm-8-dev", "llvm-8-tools", "libsdl2-dev", "libsdl2-ttf-dev", "libsdl2-image-dev", "libsdl2-mixer-dev"] sources: [*apt_sources, llvm-toolchain-xenial-8] # Finally check the compiler variants diff --git a/build-scripts/build.sh b/build-scripts/build.sh index 90ecca84bd905..0bae0c3924196 100755 --- a/build-scripts/build.sh +++ b/build-scripts/build.sh @@ -67,7 +67,16 @@ then if [ "$CATA_CLANG_TIDY" = "plugin" ] then make -j$num_jobs CataAnalyzerPlugin - CATA_CLANG_TIDY=$PWD/tools/clang-tidy-plugin/clang-tidy-plugin-support/bin/clang-tidy + export PATH=$PWD/tools/clang-tidy-plugin/clang-tidy-plugin-support/bin:$PATH + if ! which FileCheck + then + ls -l tools/clang-tidy-plugin/clang-tidy-plugin-support/bin + ls -l /usr/bin + echo "Missing FileCheck" + exit 1 + fi + CATA_CLANG_TIDY=clang-tidy + lit -v tools/clang-tidy-plugin/test fi "$CATA_CLANG_TIDY" --version diff --git a/build-scripts/requirements.sh b/build-scripts/requirements.sh index bfabcca6c602e..327463af1854a 100644 --- a/build-scripts/requirements.sh +++ b/build-scripts/requirements.sh @@ -30,7 +30,7 @@ if [ -n "${CODE_COVERAGE}" ]; then fi if [ -n "$CATA_CLANG_TIDY" ]; then - travis_retry pip install --user compiledb + travis_retry pip install --user compiledb lit fi # Influenced by https://github.com/zer0main/battleship/blob/master/build/windows/requirements.sh diff --git a/tools/clang-tidy-plugin/CMakeLists.txt b/tools/clang-tidy-plugin/CMakeLists.txt index ab576164d8777..30dab7b30abce 100644 --- a/tools/clang-tidy-plugin/CMakeLists.txt +++ b/tools/clang-tidy-plugin/CMakeLists.txt @@ -49,3 +49,6 @@ else() target_compile_options( CataAnalyzerPlugin PRIVATE -fno-exceptions -fno-rtti) endif() + +configure_file(test/lit.site.cfg.in test/lit.site.cfg @ONLY) +configure_file(test/.clang-tidy test/.clang-tidy COPYONLY) diff --git a/tools/clang-tidy-plugin/test/.clang-tidy b/tools/clang-tidy-plugin/test/.clang-tidy new file mode 100644 index 0000000000000..6179a0c5c0836 --- /dev/null +++ b/tools/clang-tidy-plugin/test/.clang-tidy @@ -0,0 +1 @@ +WarningsAsErrors: '-*' diff --git a/tools/clang-tidy-plugin/test/lit.cfg b/tools/clang-tidy-plugin/test/lit.cfg new file mode 100644 index 0000000000000..8bb0b760a4531 --- /dev/null +++ b/tools/clang-tidy-plugin/test/lit.cfg @@ -0,0 +1,23 @@ +import sys +import os +import lit.formats + +config.name = 'clang-tidy-cata' +config.test_format = lit.formats.ShTest(True) +config.test_source_root = os.path.dirname(__file__) +config.test_exec_root = os.path.join( + config.plugin_build_root, 'test') + +config.suffixes = ['.cpp'] + +check_clang_tidy = os.getenv('CHECK_CLANG_TIDY') +if not check_clang_tidy: + check_clang_tidy = os.path.join( + config.plugin_build_root, 'clang-tidy-plugin-support', 'bin', + 'check_clang_tidy.py') + +cata_plugin = os.path.join( + config.plugin_build_root, 'libCataAnalyzerPlugin.so') + +config.substitutions.append(('%check_clang_tidy', check_clang_tidy)) +config.substitutions.append(('%cata_plugin', cata_plugin)) diff --git a/tools/clang-tidy-plugin/test/lit.site.cfg.in b/tools/clang-tidy-plugin/test/lit.site.cfg.in new file mode 100644 index 0000000000000..eb6646f79c027 --- /dev/null +++ b/tools/clang-tidy-plugin/test/lit.site.cfg.in @@ -0,0 +1,6 @@ +import os + +config.plugin_build_root = "@CMAKE_CURRENT_BINARY_DIR@" + +lit_config.load_config( + config, os.path.join("@CMAKE_CURRENT_SOURCE_DIR@", "test", "lit.cfg")) diff --git a/tools/clang-tidy-plugin/test/no-long.cpp b/tools/clang-tidy-plugin/test/no-long.cpp new file mode 100644 index 0000000000000..a286c9d826408 --- /dev/null +++ b/tools/clang-tidy-plugin/test/no-long.cpp @@ -0,0 +1,4 @@ +// RUN: %check_clang_tidy %s cata-no-long %t -- -plugins=%cata_plugin -- + +long a; +// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: Variable 'a' declared as long. Prefer int or int64_t. [cata-no-long] From 41f40866dc4cc73b07eb1db142d229a379d0be97 Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Tue, 28 May 2019 22:13:19 +0100 Subject: [PATCH 08/28] Use CMake option rather than env var To find non-standard check_clang_tidy.py, was previously using an environment variable. Switch to a CMake option instead, so that it's not necessary to set the env var wherever you run the plugins tests. --- CMakeLists.txt | 3 ++- tools/clang-tidy-plugin/test/lit.cfg | 5 +++-- tools/clang-tidy-plugin/test/lit.site.cfg.in | 1 + 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index ee10e65d452be..24d62f1df2b4f 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -20,7 +20,8 @@ option(LOCALIZE "Support for language localizations. Also enable UTF support option(LANGUAGES "Compile localization files for specified languages." "" ) option(DYNAMIC_LINKING "Use dynamic linking. Or use static to remove MinGW dependency instead." "ON") option(CATA_CLANG_TIDY_PLUGIN "Build Cata's custom clang-tidy plugin" "OFF") -set(LLVM_INCLUDE_DIRS "" CACHE STRING "Paths to llvm and clang headers used for clang-tidy plugin") +set(CATA_CLANG_TIDY_INCLUDE_DIR "" CACHE STRING "Path to internal clang-tidy headers required for plugin (e.g. ClangTidy.h)") +set(CATA_CHECK_CLANG_TIDY "" CACHE STRING "Path to check_clang_tidy.py for plugin tests") set(GIT_BINARY "" CACHE STRING "Git binary name or path.") set(PREFIX "" CACHE STRING "Location of Data & GFX directories") diff --git a/tools/clang-tidy-plugin/test/lit.cfg b/tools/clang-tidy-plugin/test/lit.cfg index 8bb0b760a4531..bf660481acf60 100644 --- a/tools/clang-tidy-plugin/test/lit.cfg +++ b/tools/clang-tidy-plugin/test/lit.cfg @@ -10,8 +10,9 @@ config.test_exec_root = os.path.join( config.suffixes = ['.cpp'] -check_clang_tidy = os.getenv('CHECK_CLANG_TIDY') -if not check_clang_tidy: +if config.cata_check_clang_tidy: + check_clang_tidy = config.cata_check_clang_tidy +else: check_clang_tidy = os.path.join( config.plugin_build_root, 'clang-tidy-plugin-support', 'bin', 'check_clang_tidy.py') diff --git a/tools/clang-tidy-plugin/test/lit.site.cfg.in b/tools/clang-tidy-plugin/test/lit.site.cfg.in index eb6646f79c027..aae4d01870d78 100644 --- a/tools/clang-tidy-plugin/test/lit.site.cfg.in +++ b/tools/clang-tidy-plugin/test/lit.site.cfg.in @@ -1,6 +1,7 @@ import os config.plugin_build_root = "@CMAKE_CURRENT_BINARY_DIR@" +config.cata_check_clang_tidy = "@CATA_CHECK_CLANG_TIDY@" lit_config.load_config( config, os.path.join("@CMAKE_CURRENT_SOURCE_DIR@", "test", "lit.cfg")) From fa8f41f9a27bd8718425e923c9b445942549b82b Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Tue, 28 May 2019 22:30:50 +0100 Subject: [PATCH 09/28] Add some docs regarding how to use the plugin --- doc/DEVELOPER_TOOLING.md | 50 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/doc/DEVELOPER_TOOLING.md b/doc/DEVELOPER_TOOLING.md index da14ff0b69237..64d5e39087194 100644 --- a/doc/DEVELOPER_TOOLING.md +++ b/doc/DEVELOPER_TOOLING.md @@ -9,6 +9,56 @@ On Windows, there is an astyle extension for Visual Studio. See the [JSON style guide](JSON_STYLE.md). +## clang-tidy + +Cataclysm has a [clang-tidy configuration file](../.clang-tidy) and if you have +`clang-tidy` available you can run it to perform static analysis of the +codebase. We test with `clang-tidy` from LLVM 8.0.1 on Travis, so for the most +consistent results, you might want to use that version. + +### Custom clang-tidy plugin + +We have written our own clang-tidy checks in a custom plugin. Unfortunately, +`clang-tidy` as distributed by LLVM doesn't support plugins, so making this +work requires some extra steps. + +If you are on Ubuntu Xenial then you might be able to get it working the same +way Travis does. Add the LLVM 8 Xenial source [listed +here](https://apt.llvm.org/) to your `sources.list`, install the `clang-8 +libclang-8-dev llvm-8-dev llvm-8-tools` packages and build Cataclysm with CMake +adding `-DCATA_CLANG_TIDY_PLUGIN=ON`. + +On other distributions you will probably need to build `clang-tidy` yourself. +* Check out the `llvm`, `clang`, and `clang-tools-extra` repositories in the + required layout (as described for example + [here](https://quuxplusone.github.io/blog/2018/04/16/building-llvm-from-source/). +* Patch in plugin support for `clang-tidy` using [this + patch](https://github.com/jbytheway/clang-tidy-plugin-support/blob/master/plugin-support.patch). +* Configure LLVM using CMake, including the + `-DCMAKE_EXE_LINKER_FLAGS="-rdynamic"` option. +* Add the `build/bin` directory to your path so that `clang-tidy` and + `FileCheck` are found from there. + +Then you can use your locally build `clang-tidy` to compile Cataclysm. You'll +need to use the CMake version of the Cataclysm build rather than the `Makefile` +build. Add the following CMake options: +```sh +-DCATA_CLANG_TIDY_PLUGIN=ON +-DCATA_CLANG_TIDY_INCLUDE_DIR="$extra_dir/clang-tidy" +-DCATA_CHECK_CLANG_TIDY="$extra_dir/test/clang-tidy/check_clang_tidy.py" +``` +where `$extra_dir` is the location of your `clang-tools-extra` checkout. + +If you wish to run the tests for the custom clang-tidy plugin you will also +need `lit`. This will be built as part of `llvm`, or you can install it via +`pip` or your local package manager if you prefer. + +Then, assuming `build` is your Cataclysm build directory, you can run the tests +with +```sh +lit -v build/tools/clang-tidy-plugin/test +``` + ## include-what-you-use [include-what-you-use](https://github.com/include-what-you-use/include-what-you-use) From 8c9108ede7d48351ff480824cb45758d96263a1e Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Sat, 8 Jun 2019 13:30:29 +0100 Subject: [PATCH 10/28] Also catch unsigned long in cata-no-long check --- tools/clang-tidy-plugin/NoLongCheck.cpp | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/tools/clang-tidy-plugin/NoLongCheck.cpp b/tools/clang-tidy-plugin/NoLongCheck.cpp index 864a6dc1e2d94..8e92d77036b52 100644 --- a/tools/clang-tidy-plugin/NoLongCheck.cpp +++ b/tools/clang-tidy-plugin/NoLongCheck.cpp @@ -10,15 +10,22 @@ namespace cata { void NoLongCheck::registerMatchers(MatchFinder *Finder) { Finder->addMatcher(valueDecl(hasType(asString("long"))).bind("decl"), this); + Finder->addMatcher(valueDecl(hasType(asString("unsigned long"))).bind("decl"), this); } void NoLongCheck::check(const MatchFinder::MatchResult &Result) { - const auto *MatchedDecl = Result.Nodes.getNodeAs("decl"); + const ValueDecl *MatchedDecl = Result.Nodes.getNodeAs("decl"); if( !MatchedDecl || !MatchedDecl->getLocation().isValid() ) { return; } - diag(MatchedDecl->getLocation(), "Variable %0 declared as long. Prefer int or int64_t.") - << MatchedDecl; + QualType Type = MatchedDecl->getType().getUnqualifiedType(); + if( Type.getAsString() == "long" ) { + diag(MatchedDecl->getLocation(), "Variable %0 declared as long. Prefer int or int64_t.") + << MatchedDecl; + } else { + diag(MatchedDecl->getLocation(), "Variable %0 declared as unsigned long. " + "Prefer unsigned int or uint64_t.") << MatchedDecl; + } } } // namespace cata From 85a19076a8590d825f47e2aeccef621fb74c36cd Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Sat, 8 Jun 2019 13:30:56 +0100 Subject: [PATCH 11/28] More tests for cata-no-long check --- tools/clang-tidy-plugin/test/no-long.cpp | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/tools/clang-tidy-plugin/test/no-long.cpp b/tools/clang-tidy-plugin/test/no-long.cpp index a286c9d826408..6064861a2670c 100644 --- a/tools/clang-tidy-plugin/test/no-long.cpp +++ b/tools/clang-tidy-plugin/test/no-long.cpp @@ -1,4 +1,15 @@ // RUN: %check_clang_tidy %s cata-no-long %t -- -plugins=%cata_plugin -- +#include + long a; -// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: Variable 'a' declared as long. Prefer int or int64_t. [cata-no-long] +// CHECK-MESSAGES: warning: Variable 'a' declared as long. Prefer int or int64_t. [cata-no-long] + +unsigned long b; +// CHECK-MESSAGES: warning: Variable 'b' declared as unsigned long. Prefer unsigned int or uint64_t. [cata-no-long] + +int64_t c; +uint64_t d; + +void f(long e); +// CHECK-MESSAGES: warning: Variable 'e' declared as long. Prefer int or int64_t. [cata-no-long] From 85d7b9f1098f066fd832ac8d5be502e4a626d92e Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Sat, 8 Jun 2019 13:51:43 +0100 Subject: [PATCH 12/28] Check function return types are not long --- tools/clang-tidy-plugin/NoLongCheck.cpp | 54 ++++++++++++++++++------ tools/clang-tidy-plugin/test/no-long.cpp | 5 +++ 2 files changed, 45 insertions(+), 14 deletions(-) diff --git a/tools/clang-tidy-plugin/NoLongCheck.cpp b/tools/clang-tidy-plugin/NoLongCheck.cpp index 8e92d77036b52..a1758233cdd06 100644 --- a/tools/clang-tidy-plugin/NoLongCheck.cpp +++ b/tools/clang-tidy-plugin/NoLongCheck.cpp @@ -9,23 +9,49 @@ namespace tidy { namespace cata { void NoLongCheck::registerMatchers(MatchFinder *Finder) { - Finder->addMatcher(valueDecl(hasType(asString("long"))).bind("decl"), this); - Finder->addMatcher(valueDecl(hasType(asString("unsigned long"))).bind("decl"), this); + using TypeMatcher = clang::ast_matchers::internal::Matcher; + const TypeMatcher isLong = anyOf(asString("long"), asString("unsigned long")); + Finder->addMatcher(valueDecl(hasType(isLong)).bind("decl"), this); + Finder->addMatcher(functionDecl(returns(isLong)).bind("return"), this); +} + +static void CheckDecl(NoLongCheck &Check, const MatchFinder::MatchResult &Result) { + const ValueDecl *MatchedDecl = Result.Nodes.getNodeAs("decl"); + if( !MatchedDecl || !MatchedDecl->getLocation().isValid() ) { + return; + } + QualType Type = MatchedDecl->getType().getUnqualifiedType(); + if( Type.getAsString() == "long" ) { + Check.diag( + MatchedDecl->getLocation(), "Variable %0 declared as long. " + "Prefer int or int64_t.") << MatchedDecl; + } else { + Check.diag( + MatchedDecl->getLocation(), "Variable %0 declared as unsigned long. " + "Prefer unsigned int or uint64_t.") << MatchedDecl; + } +} + +static void CheckReturn(NoLongCheck &Check, const MatchFinder::MatchResult &Result) { + const FunctionDecl *MatchedDecl = Result.Nodes.getNodeAs("return"); + if( !MatchedDecl || !MatchedDecl->getLocation().isValid() ) { + return; + } + QualType Type = MatchedDecl->getReturnType().getUnqualifiedType(); + if( Type.getAsString() == "long" ) { + Check.diag( + MatchedDecl->getLocation(), "Function %0 declared as returning long. " + "Prefer int or int64_t.") << MatchedDecl; + } else { + Check.diag( + MatchedDecl->getLocation(), "Function %0 declared as returning unsigned long. " + "Prefer unsigned int or uint64_t.") << MatchedDecl; + } } void NoLongCheck::check(const MatchFinder::MatchResult &Result) { - const ValueDecl *MatchedDecl = Result.Nodes.getNodeAs("decl"); - if( !MatchedDecl || !MatchedDecl->getLocation().isValid() ) { - return; - } - QualType Type = MatchedDecl->getType().getUnqualifiedType(); - if( Type.getAsString() == "long" ) { - diag(MatchedDecl->getLocation(), "Variable %0 declared as long. Prefer int or int64_t.") - << MatchedDecl; - } else { - diag(MatchedDecl->getLocation(), "Variable %0 declared as unsigned long. " - "Prefer unsigned int or uint64_t.") << MatchedDecl; - } + CheckDecl(*this, Result); + CheckReturn(*this, Result); } } // namespace cata diff --git a/tools/clang-tidy-plugin/test/no-long.cpp b/tools/clang-tidy-plugin/test/no-long.cpp index 6064861a2670c..0178985dd6bbf 100644 --- a/tools/clang-tidy-plugin/test/no-long.cpp +++ b/tools/clang-tidy-plugin/test/no-long.cpp @@ -13,3 +13,8 @@ uint64_t d; void f(long e); // CHECK-MESSAGES: warning: Variable 'e' declared as long. Prefer int or int64_t. [cata-no-long] + +long g(); +// CHECK-MESSAGES: warning: Function 'g' declared as returning long. Prefer int or int64_t. [cata-no-long] + +int64_t h(); From 65ac6f89e70603b7d909770263d69f0dbc571f48 Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Sat, 8 Jun 2019 14:13:49 +0100 Subject: [PATCH 13/28] Tweak function tests for cata-no-long --- tools/clang-tidy-plugin/test/no-long.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tools/clang-tidy-plugin/test/no-long.cpp b/tools/clang-tidy-plugin/test/no-long.cpp index 0178985dd6bbf..fbc8e3f26713b 100644 --- a/tools/clang-tidy-plugin/test/no-long.cpp +++ b/tools/clang-tidy-plugin/test/no-long.cpp @@ -11,10 +11,11 @@ unsigned long b; int64_t c; uint64_t d; -void f(long e); +void f1(long e); // CHECK-MESSAGES: warning: Variable 'e' declared as long. Prefer int or int64_t. [cata-no-long] -long g(); -// CHECK-MESSAGES: warning: Function 'g' declared as returning long. Prefer int or int64_t. [cata-no-long] +long f2(); +// CHECK-MESSAGES: warning: Function 'f2' declared as returning long. Prefer int or int64_t. [cata-no-long] -int64_t h(); +int64_t f3(); +auto f4() -> decltype(0L); From 6b8be8446a63ac7041a36e80a5ad44d8f1a617e0 Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Sat, 8 Jun 2019 14:14:10 +0100 Subject: [PATCH 14/28] Check for static_cast in cata-no-long --- tools/clang-tidy-plugin/NoLongCheck.cpp | 19 +++++++++++++++++++ tools/clang-tidy-plugin/test/no-long.cpp | 3 +++ 2 files changed, 22 insertions(+) diff --git a/tools/clang-tidy-plugin/NoLongCheck.cpp b/tools/clang-tidy-plugin/NoLongCheck.cpp index a1758233cdd06..38c96ef93945a 100644 --- a/tools/clang-tidy-plugin/NoLongCheck.cpp +++ b/tools/clang-tidy-plugin/NoLongCheck.cpp @@ -13,6 +13,7 @@ void NoLongCheck::registerMatchers(MatchFinder *Finder) { const TypeMatcher isLong = anyOf(asString("long"), asString("unsigned long")); Finder->addMatcher(valueDecl(hasType(isLong)).bind("decl"), this); Finder->addMatcher(functionDecl(returns(isLong)).bind("return"), this); + Finder->addMatcher(cxxStaticCastExpr(hasType(isLong)).bind("cast"), this); } static void CheckDecl(NoLongCheck &Check, const MatchFinder::MatchResult &Result) { @@ -49,9 +50,27 @@ static void CheckReturn(NoLongCheck &Check, const MatchFinder::MatchResult &Resu } } +static void CheckCast(NoLongCheck &Check, const MatchFinder::MatchResult &Result) { + const CXXStaticCastExpr *MatchedDecl = Result.Nodes.getNodeAs("cast"); + if( !MatchedDecl ) { + return; + } + QualType Type = MatchedDecl->getType().getUnqualifiedType(); + SourceLocation location = MatchedDecl->getTypeInfoAsWritten()->getTypeLoc().getBeginLoc(); + if( Type.getAsString() == "long" ) { + Check.diag( + location, "Static cast to long. Prefer int or int64_t."); + } else { + Check.diag( + location, "Static cast to unsigned long. " + "Prefer unsigned int or uint64_t."); + } +} + void NoLongCheck::check(const MatchFinder::MatchResult &Result) { CheckDecl(*this, Result); CheckReturn(*this, Result); + CheckCast(*this, Result); } } // namespace cata diff --git a/tools/clang-tidy-plugin/test/no-long.cpp b/tools/clang-tidy-plugin/test/no-long.cpp index fbc8e3f26713b..15d33463a0267 100644 --- a/tools/clang-tidy-plugin/test/no-long.cpp +++ b/tools/clang-tidy-plugin/test/no-long.cpp @@ -19,3 +19,6 @@ long f2(); int64_t f3(); auto f4() -> decltype(0L); + +int i1 = static_cast(0); +int i2 = static_cast(0); From d688b0d53b36b2f54e7756def31537ed34e6586b Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Sat, 8 Jun 2019 22:04:31 +0100 Subject: [PATCH 15/28] cata-no-long: Handle qualified and reference types Previously we would not flag uses of long if cv-qualified or a reference type. Now we will. --- tools/clang-tidy-plugin/NoLongCheck.cpp | 68 +++++++++++++----------- tools/clang-tidy-plugin/test/no-long.cpp | 19 ++++--- 2 files changed, 50 insertions(+), 37 deletions(-) diff --git a/tools/clang-tidy-plugin/NoLongCheck.cpp b/tools/clang-tidy-plugin/NoLongCheck.cpp index 38c96ef93945a..ffa7451fc27f9 100644 --- a/tools/clang-tidy-plugin/NoLongCheck.cpp +++ b/tools/clang-tidy-plugin/NoLongCheck.cpp @@ -10,10 +10,23 @@ namespace cata { void NoLongCheck::registerMatchers(MatchFinder *Finder) { using TypeMatcher = clang::ast_matchers::internal::Matcher; - const TypeMatcher isLong = anyOf(asString("long"), asString("unsigned long")); - Finder->addMatcher(valueDecl(hasType(isLong)).bind("decl"), this); - Finder->addMatcher(functionDecl(returns(isLong)).bind("return"), this); - Finder->addMatcher(cxxStaticCastExpr(hasType(isLong)).bind("cast"), this); + const TypeMatcher isIntegerOrRef = anyOf(isInteger(), references(isInteger())); + Finder->addMatcher(valueDecl(hasType(isIntegerOrRef)).bind("decl"), this); + Finder->addMatcher(functionDecl(returns(isIntegerOrRef)).bind("return"), this); + Finder->addMatcher(cxxStaticCastExpr(hasDestinationType(isIntegerOrRef)).bind("cast"), this); +} + +static std::string AlternativesFor( QualType Type ) { + Type = Type.getNonReferenceType(); + Type = Type.getLocalUnqualifiedType(); + std::string name = Type.getAsString(); + if( name == "long" ) { + return "Prefer int or int64_t to long"; + } else if( name == "unsigned long" ) { + return "Prefer unsigned int or uint64_t to unsigned long"; + } else { + return {}; + } } static void CheckDecl(NoLongCheck &Check, const MatchFinder::MatchResult &Result) { @@ -21,16 +34,14 @@ static void CheckDecl(NoLongCheck &Check, const MatchFinder::MatchResult &Result if( !MatchedDecl || !MatchedDecl->getLocation().isValid() ) { return; } - QualType Type = MatchedDecl->getType().getUnqualifiedType(); - if( Type.getAsString() == "long" ) { - Check.diag( - MatchedDecl->getLocation(), "Variable %0 declared as long. " - "Prefer int or int64_t.") << MatchedDecl; - } else { - Check.diag( - MatchedDecl->getLocation(), "Variable %0 declared as unsigned long. " - "Prefer unsigned int or uint64_t.") << MatchedDecl; + QualType Type = MatchedDecl->getType(); + std::string alternatives = AlternativesFor(Type); + if( alternatives.empty() ) { + return; } + Check.diag( + MatchedDecl->getLocation(), "Variable %0 declared as %1. %2.") << + MatchedDecl << Type << alternatives; } static void CheckReturn(NoLongCheck &Check, const MatchFinder::MatchResult &Result) { @@ -38,16 +49,14 @@ static void CheckReturn(NoLongCheck &Check, const MatchFinder::MatchResult &Resu if( !MatchedDecl || !MatchedDecl->getLocation().isValid() ) { return; } - QualType Type = MatchedDecl->getReturnType().getUnqualifiedType(); - if( Type.getAsString() == "long" ) { - Check.diag( - MatchedDecl->getLocation(), "Function %0 declared as returning long. " - "Prefer int or int64_t.") << MatchedDecl; - } else { - Check.diag( - MatchedDecl->getLocation(), "Function %0 declared as returning unsigned long. " - "Prefer unsigned int or uint64_t.") << MatchedDecl; + QualType Type = MatchedDecl->getReturnType(); + std::string alternatives = AlternativesFor(Type); + if( alternatives.empty() ) { + return; } + Check.diag( + MatchedDecl->getLocation(), "Function %0 declared as returning %1. %2.") << + MatchedDecl << Type << alternatives; } static void CheckCast(NoLongCheck &Check, const MatchFinder::MatchResult &Result) { @@ -55,16 +64,13 @@ static void CheckCast(NoLongCheck &Check, const MatchFinder::MatchResult &Result if( !MatchedDecl ) { return; } - QualType Type = MatchedDecl->getType().getUnqualifiedType(); - SourceLocation location = MatchedDecl->getTypeInfoAsWritten()->getTypeLoc().getBeginLoc(); - if( Type.getAsString() == "long" ) { - Check.diag( - location, "Static cast to long. Prefer int or int64_t."); - } else { - Check.diag( - location, "Static cast to unsigned long. " - "Prefer unsigned int or uint64_t."); + QualType Type = MatchedDecl->getType(); + std::string alternatives = AlternativesFor(Type); + if( alternatives.empty() ) { + return; } + SourceLocation location = MatchedDecl->getTypeInfoAsWritten()->getTypeLoc().getBeginLoc(); + Check.diag( location, "Static cast to %0. %1.") << Type << alternatives; } void NoLongCheck::check(const MatchFinder::MatchResult &Result) { diff --git a/tools/clang-tidy-plugin/test/no-long.cpp b/tools/clang-tidy-plugin/test/no-long.cpp index 15d33463a0267..4fe75a15ec074 100644 --- a/tools/clang-tidy-plugin/test/no-long.cpp +++ b/tools/clang-tidy-plugin/test/no-long.cpp @@ -2,23 +2,30 @@ #include -long a; -// CHECK-MESSAGES: warning: Variable 'a' declared as long. Prefer int or int64_t. [cata-no-long] +long a1; +// CHECK-MESSAGES: warning: Variable 'a1' declared as 'long'. Prefer int or int64_t to long. [cata-no-long] -unsigned long b; -// CHECK-MESSAGES: warning: Variable 'b' declared as unsigned long. Prefer unsigned int or uint64_t. [cata-no-long] +unsigned long a2; +// CHECK-MESSAGES: warning: Variable 'a2' declared as 'unsigned long'. Prefer unsigned int or uint64_t to unsigned long. [cata-no-long] + +const long a3 = 0; +// CHECK-MESSAGES: warning: Variable 'a3' declared as 'const long'. Prefer int or int64_t to long. [cata-no-long] + +long &a4 = a1; +// CHECK-MESSAGES: warning: Variable 'a4' declared as 'long &'. Prefer int or int64_t to long. [cata-no-long] int64_t c; uint64_t d; void f1(long e); -// CHECK-MESSAGES: warning: Variable 'e' declared as long. Prefer int or int64_t. [cata-no-long] +// CHECK-MESSAGES: warning: Variable 'e' declared as 'long'. Prefer int or int64_t to long. [cata-no-long] long f2(); -// CHECK-MESSAGES: warning: Function 'f2' declared as returning long. Prefer int or int64_t. [cata-no-long] +// CHECK-MESSAGES: warning: Function 'f2' declared as returning 'long'. Prefer int or int64_t to long. [cata-no-long] int64_t f3(); auto f4() -> decltype(0L); int i1 = static_cast(0); +// CHECK-MESSAGES: warning: Static cast to 'long'. Prefer int or int64_t to long. [cata-no-long] int i2 = static_cast(0); From adc3f21b59c18639d892f4ec740b04fe7a6dc08b Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Sun, 9 Jun 2019 20:15:51 +0100 Subject: [PATCH 16/28] Astyle clang-tidy plugin code --- tools/clang-tidy-plugin/CataTidyModule.cpp | 22 +++++--- tools/clang-tidy-plugin/NoLongCheck.cpp | 64 +++++++++++++--------- tools/clang-tidy-plugin/NoLongCheck.h | 22 +++++--- tools/clang-tidy-plugin/test/no-long.cpp | 8 +-- 4 files changed, 67 insertions(+), 49 deletions(-) diff --git a/tools/clang-tidy-plugin/CataTidyModule.cpp b/tools/clang-tidy-plugin/CataTidyModule.cpp index 16248a6554616..34e78038051c0 100644 --- a/tools/clang-tidy-plugin/CataTidyModule.cpp +++ b/tools/clang-tidy-plugin/CataTidyModule.cpp @@ -3,22 +3,26 @@ #include "ClangTidyModuleRegistry.h" #include "NoLongCheck.h" -namespace clang { -namespace tidy { -namespace cata { +namespace clang +{ +namespace tidy +{ +namespace cata +{ -class CataModule : public ClangTidyModule { -public: - void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { - CheckFactories.registerCheck( "cata-no-long" ); - } +class CataModule : public ClangTidyModule +{ + public: + void addCheckFactories( ClangTidyCheckFactories &CheckFactories ) override { + CheckFactories.registerCheck( "cata-no-long" ); + } }; } // namespace cata // Register the MiscTidyModule using this statically initialized variable. static ClangTidyModuleRegistry::Add - X("cata-module", "Adds Cataclysm-DDA checks."); +X( "cata-module", "Adds Cataclysm-DDA checks." ); } // namespace tidy } // namespace clang diff --git a/tools/clang-tidy-plugin/NoLongCheck.cpp b/tools/clang-tidy-plugin/NoLongCheck.cpp index ffa7451fc27f9..c5d4feddb8222 100644 --- a/tools/clang-tidy-plugin/NoLongCheck.cpp +++ b/tools/clang-tidy-plugin/NoLongCheck.cpp @@ -4,19 +4,25 @@ using namespace clang::ast_matchers; -namespace clang { -namespace tidy { -namespace cata { +namespace clang +{ +namespace tidy +{ +namespace cata +{ -void NoLongCheck::registerMatchers(MatchFinder *Finder) { +void NoLongCheck::registerMatchers( MatchFinder *Finder ) +{ using TypeMatcher = clang::ast_matchers::internal::Matcher; - const TypeMatcher isIntegerOrRef = anyOf(isInteger(), references(isInteger())); - Finder->addMatcher(valueDecl(hasType(isIntegerOrRef)).bind("decl"), this); - Finder->addMatcher(functionDecl(returns(isIntegerOrRef)).bind("return"), this); - Finder->addMatcher(cxxStaticCastExpr(hasDestinationType(isIntegerOrRef)).bind("cast"), this); + const TypeMatcher isIntegerOrRef = anyOf( isInteger(), references( isInteger() ) ); + Finder->addMatcher( valueDecl( hasType( isIntegerOrRef ) ).bind( "decl" ), this ); + Finder->addMatcher( functionDecl( returns( isIntegerOrRef ) ).bind( "return" ), this ); + Finder->addMatcher( cxxStaticCastExpr( hasDestinationType( isIntegerOrRef ) ).bind( "cast" ), + this ); } -static std::string AlternativesFor( QualType Type ) { +static std::string AlternativesFor( QualType Type ) +{ Type = Type.getNonReferenceType(); Type = Type.getLocalUnqualifiedType(); std::string name = Type.getAsString(); @@ -29,54 +35,58 @@ static std::string AlternativesFor( QualType Type ) { } } -static void CheckDecl(NoLongCheck &Check, const MatchFinder::MatchResult &Result) { - const ValueDecl *MatchedDecl = Result.Nodes.getNodeAs("decl"); +static void CheckDecl( NoLongCheck &Check, const MatchFinder::MatchResult &Result ) +{ + const ValueDecl *MatchedDecl = Result.Nodes.getNodeAs( "decl" ); if( !MatchedDecl || !MatchedDecl->getLocation().isValid() ) { return; } QualType Type = MatchedDecl->getType(); - std::string alternatives = AlternativesFor(Type); + std::string alternatives = AlternativesFor( Type ); if( alternatives.empty() ) { return; } Check.diag( - MatchedDecl->getLocation(), "Variable %0 declared as %1. %2.") << - MatchedDecl << Type << alternatives; + MatchedDecl->getLocation(), "Variable %0 declared as %1. %2." ) << + MatchedDecl << Type << alternatives; } -static void CheckReturn(NoLongCheck &Check, const MatchFinder::MatchResult &Result) { - const FunctionDecl *MatchedDecl = Result.Nodes.getNodeAs("return"); +static void CheckReturn( NoLongCheck &Check, const MatchFinder::MatchResult &Result ) +{ + const FunctionDecl *MatchedDecl = Result.Nodes.getNodeAs( "return" ); if( !MatchedDecl || !MatchedDecl->getLocation().isValid() ) { return; } QualType Type = MatchedDecl->getReturnType(); - std::string alternatives = AlternativesFor(Type); + std::string alternatives = AlternativesFor( Type ); if( alternatives.empty() ) { return; } Check.diag( - MatchedDecl->getLocation(), "Function %0 declared as returning %1. %2.") << - MatchedDecl << Type << alternatives; + MatchedDecl->getLocation(), "Function %0 declared as returning %1. %2." ) << + MatchedDecl << Type << alternatives; } -static void CheckCast(NoLongCheck &Check, const MatchFinder::MatchResult &Result) { - const CXXStaticCastExpr *MatchedDecl = Result.Nodes.getNodeAs("cast"); +static void CheckCast( NoLongCheck &Check, const MatchFinder::MatchResult &Result ) +{ + const CXXStaticCastExpr *MatchedDecl = Result.Nodes.getNodeAs( "cast" ); if( !MatchedDecl ) { return; } QualType Type = MatchedDecl->getType(); - std::string alternatives = AlternativesFor(Type); + std::string alternatives = AlternativesFor( Type ); if( alternatives.empty() ) { return; } SourceLocation location = MatchedDecl->getTypeInfoAsWritten()->getTypeLoc().getBeginLoc(); - Check.diag( location, "Static cast to %0. %1.") << Type << alternatives; + Check.diag( location, "Static cast to %0. %1." ) << Type << alternatives; } -void NoLongCheck::check(const MatchFinder::MatchResult &Result) { - CheckDecl(*this, Result); - CheckReturn(*this, Result); - CheckCast(*this, Result); +void NoLongCheck::check( const MatchFinder::MatchResult &Result ) +{ + CheckDecl( *this, Result ); + CheckReturn( *this, Result ); + CheckCast( *this, Result ); } } // namespace cata diff --git a/tools/clang-tidy-plugin/NoLongCheck.h b/tools/clang-tidy-plugin/NoLongCheck.h index caa388b7b5deb..80e4fe11b95fc 100644 --- a/tools/clang-tidy-plugin/NoLongCheck.h +++ b/tools/clang-tidy-plugin/NoLongCheck.h @@ -3,16 +3,20 @@ #include "ClangTidy.h" -namespace clang { -namespace tidy { -namespace cata { +namespace clang +{ +namespace tidy +{ +namespace cata +{ -class NoLongCheck : public ClangTidyCheck { -public: - NoLongCheck(StringRef Name, ClangTidyContext *Context) - : ClangTidyCheck(Name, Context) {} - void registerMatchers(ast_matchers::MatchFinder *Finder) override; - void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +class NoLongCheck : public ClangTidyCheck +{ + public: + NoLongCheck( StringRef Name, ClangTidyContext *Context ) + : ClangTidyCheck( Name, Context ) {} + void registerMatchers( ast_matchers::MatchFinder *Finder ) override; + void check( const ast_matchers::MatchFinder::MatchResult &Result ) override; }; } // namespace cata diff --git a/tools/clang-tidy-plugin/test/no-long.cpp b/tools/clang-tidy-plugin/test/no-long.cpp index 4fe75a15ec074..78cea77e738ae 100644 --- a/tools/clang-tidy-plugin/test/no-long.cpp +++ b/tools/clang-tidy-plugin/test/no-long.cpp @@ -17,15 +17,15 @@ long &a4 = a1; int64_t c; uint64_t d; -void f1(long e); +void f1( long e ); // CHECK-MESSAGES: warning: Variable 'e' declared as 'long'. Prefer int or int64_t to long. [cata-no-long] long f2(); // CHECK-MESSAGES: warning: Function 'f2' declared as returning 'long'. Prefer int or int64_t to long. [cata-no-long] int64_t f3(); -auto f4() -> decltype(0L); +auto f4() -> decltype( 0L ); -int i1 = static_cast(0); +int i1 = static_cast( 0 ); // CHECK-MESSAGES: warning: Static cast to 'long'. Prefer int or int64_t to long. [cata-no-long] -int i2 = static_cast(0); +int i2 = static_cast( 0 ); From 30287ab427490f4117e88b941cb1179f831385fd Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Sun, 9 Jun 2019 21:33:14 +0100 Subject: [PATCH 17/28] Enforce astyle on clang-tidy plugin code --- Makefile | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Makefile b/Makefile index a2b335ea34ade..55607bf03a2ee 100644 --- a/Makefile +++ b/Makefile @@ -685,6 +685,8 @@ TESTSRC := $(wildcard tests/*.cpp) TESTHDR := $(wildcard tests/*.h) JSON_FORMATTER_SOURCES := tools/format/format.cpp src/json.cpp CHKJSON_SOURCES := src/chkjson/chkjson.cpp src/json.cpp +CLANG_TIDY_PLUGIN_SOURCES := \ + $(wildcard tools/clang-tidy-plugin/*.cpp tools/clang-tidy-plugin/*/*.cpp) TOOLHDR := $(wildcard tools/*/*.h) # Using sort here because it has the side-effect of deduplicating the list ASTYLE_SOURCES := $(sort \ @@ -694,6 +696,7 @@ ASTYLE_SOURCES := $(sort \ $(TESTHDR) \ $(JSON_FORMATTER_SOURCES) \ $(CHKJSON_SOURCES) \ + $(CLANG_TIDY_PLUGIN_SOURCES) \ $(TOOLHDR)) _OBJS = $(SOURCES:$(SRC_DIR)/%.cpp=%.o) From 9eb11d1158ab547482b53a6c45b42fc92f1b54ce Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Mon, 10 Jun 2019 20:10:49 +0100 Subject: [PATCH 18/28] Avoid some false positives in cata-no-long check When variables had a type which was some template parameter which happened to be long, this caused an error to be reported. We don't actually care about such cases; instead we would prefer the error to appear at the point the template argument is specified. --- tools/clang-tidy-plugin/NoLongCheck.cpp | 14 ++++++++++++++ tools/clang-tidy-plugin/test/no-long.cpp | 16 ++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/tools/clang-tidy-plugin/NoLongCheck.cpp b/tools/clang-tidy-plugin/NoLongCheck.cpp index c5d4feddb8222..1e6718d7edf06 100644 --- a/tools/clang-tidy-plugin/NoLongCheck.cpp +++ b/tools/clang-tidy-plugin/NoLongCheck.cpp @@ -46,6 +46,20 @@ static void CheckDecl( NoLongCheck &Check, const MatchFinder::MatchResult &Resul if( alternatives.empty() ) { return; } + Decl::Kind contextKind = MatchedDecl->getDeclContext()->getDeclKind(); + if( contextKind == Decl::Function || contextKind == Decl::CXXMethod || + contextKind == Decl::CXXConstructor || contextKind == Decl::CXXConversion || + contextKind == Decl::CXXDestructor || contextKind == Decl::CXXDeductionGuide ) { + TemplateSpecializationKind tsk = + static_cast( + MatchedDecl->getDeclContext() )->getTemplateSpecializationKind(); + if( tsk == TSK_ImplicitInstantiation ) { + // This happens for e.g. a parameter 'T a' to an instantiated + // template function where T is long. We don't want to report such + // cases. + return; + } + } Check.diag( MatchedDecl->getLocation(), "Variable %0 declared as %1. %2." ) << MatchedDecl << Type << alternatives; diff --git a/tools/clang-tidy-plugin/test/no-long.cpp b/tools/clang-tidy-plugin/test/no-long.cpp index 78cea77e738ae..c2cf2b2476631 100644 --- a/tools/clang-tidy-plugin/test/no-long.cpp +++ b/tools/clang-tidy-plugin/test/no-long.cpp @@ -29,3 +29,19 @@ auto f4() -> decltype( 0L ); int i1 = static_cast( 0 ); // CHECK-MESSAGES: warning: Static cast to 'long'. Prefer int or int64_t to long. [cata-no-long] int i2 = static_cast( 0 ); + +template +void g( T gp0, long gp1 ) +{ + // CHECK-MESSAGES: warning: Variable 'gp1' declared as 'long'. Prefer int or int64_t to long. [cata-no-long] + long gi0; + // CHECK-MESSAGES: warning: Variable 'gi0' declared as 'long'. Prefer int or int64_t to long. [cata-no-long] + T gi1; +} + +void h() +{ + g( 0, 0 ); + // Would like to report an error here for the template argument, but have + // not found a way to do so. +} From dce24e58e393bbcb450dfa38bc4879c824938f40 Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Mon, 10 Jun 2019 23:33:15 +0100 Subject: [PATCH 19/28] NoLongCheck now tests for long-specific macros Should not be using LONG_MIN et al if we're not using long variables. --- tools/clang-tidy-plugin/NoLongCheck.cpp | 26 ++++++++++++++++++++++++ tools/clang-tidy-plugin/NoLongCheck.h | 1 + tools/clang-tidy-plugin/test/no-long.cpp | 10 ++++++++- 3 files changed, 36 insertions(+), 1 deletion(-) diff --git a/tools/clang-tidy-plugin/NoLongCheck.cpp b/tools/clang-tidy-plugin/NoLongCheck.cpp index 1e6718d7edf06..7cf7b41d561f6 100644 --- a/tools/clang-tidy-plugin/NoLongCheck.cpp +++ b/tools/clang-tidy-plugin/NoLongCheck.cpp @@ -1,6 +1,7 @@ #include "NoLongCheck.h" #include "clang/AST/ASTContext.h" #include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Frontend/CompilerInstance.h" using namespace clang::ast_matchers; @@ -11,6 +12,31 @@ namespace tidy namespace cata { +class NoLongMacrosCallbacks : public PPCallbacks +{ + public: + NoLongMacrosCallbacks( NoLongCheck *Check ) : + Check( Check ) {} + + void MacroExpands( const Token &MacroNameTok, + const MacroDefinition &, + SourceRange Range, + const MacroArgs * ) override { + StringRef MacroName = MacroNameTok.getIdentifierInfo()->getName(); + if( MacroName == "LONG_MIN" || MacroName == "LONG_MAX" || MacroName == "ULONG_MAX" ) { + Check->diag( Range.getBegin(), "Use of long-specific macro %0" ) << MacroName; + } + } + private: + NoLongCheck *Check; +}; + +void NoLongCheck::registerPPCallbacks( CompilerInstance &Compiler ) +{ + Compiler.getPreprocessor().addPPCallbacks( + llvm::make_unique( this ) ); +} + void NoLongCheck::registerMatchers( MatchFinder *Finder ) { using TypeMatcher = clang::ast_matchers::internal::Matcher; diff --git a/tools/clang-tidy-plugin/NoLongCheck.h b/tools/clang-tidy-plugin/NoLongCheck.h index 80e4fe11b95fc..e6c6c489f2ae4 100644 --- a/tools/clang-tidy-plugin/NoLongCheck.h +++ b/tools/clang-tidy-plugin/NoLongCheck.h @@ -15,6 +15,7 @@ class NoLongCheck : public ClangTidyCheck public: NoLongCheck( StringRef Name, ClangTidyContext *Context ) : ClangTidyCheck( Name, Context ) {} + void registerPPCallbacks( CompilerInstance &Compiler ) override; void registerMatchers( ast_matchers::MatchFinder *Finder ) override; void check( const ast_matchers::MatchFinder::MatchResult &Result ) override; }; diff --git a/tools/clang-tidy-plugin/test/no-long.cpp b/tools/clang-tidy-plugin/test/no-long.cpp index c2cf2b2476631..48d9634f7a50d 100644 --- a/tools/clang-tidy-plugin/test/no-long.cpp +++ b/tools/clang-tidy-plugin/test/no-long.cpp @@ -1,5 +1,6 @@ // RUN: %check_clang_tidy %s cata-no-long %t -- -plugins=%cata_plugin -- +#include #include long a1; @@ -31,7 +32,7 @@ int i1 = static_cast( 0 ); int i2 = static_cast( 0 ); template -void g( T gp0, long gp1 ) +void g( T gp0, long gp1 = 0 ) { // CHECK-MESSAGES: warning: Variable 'gp1' declared as 'long'. Prefer int or int64_t to long. [cata-no-long] long gi0; @@ -44,4 +45,11 @@ void h() g( 0, 0 ); // Would like to report an error here for the template argument, but have // not found a way to do so. + + g( LONG_MIN ); + // CHECK-MESSAGES: warning: Use of long-specific macro LONG_MIN [cata-no-long] + g( LONG_MAX ); + // CHECK-MESSAGES: warning: Use of long-specific macro LONG_MAX [cata-no-long] + g( ULONG_MAX ); + // CHECK-MESSAGES: warning: Use of long-specific macro ULONG_MAX [cata-no-long] } From 10c463b98af811c01b3c230de84340c269596692 Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Tue, 11 Jun 2019 20:50:35 +0100 Subject: [PATCH 20/28] Avoid false-positives on return values The check was reporting errors for functions returning long when that was a template parameter. We shouldn't complain in this case. --- tools/clang-tidy-plugin/NoLongCheck.cpp | 13 +++++++++++++ tools/clang-tidy-plugin/test/no-long.cpp | 11 +++++++++++ 2 files changed, 24 insertions(+) diff --git a/tools/clang-tidy-plugin/NoLongCheck.cpp b/tools/clang-tidy-plugin/NoLongCheck.cpp index 7cf7b41d561f6..22f9728c4b529 100644 --- a/tools/clang-tidy-plugin/NoLongCheck.cpp +++ b/tools/clang-tidy-plugin/NoLongCheck.cpp @@ -102,6 +102,19 @@ static void CheckReturn( NoLongCheck &Check, const MatchFinder::MatchResult &Res if( alternatives.empty() ) { return; } + + Decl::Kind contextKind = MatchedDecl->getDeclContext()->getDeclKind(); + if( contextKind == Decl::ClassTemplateSpecialization ) { + TemplateSpecializationKind tsk = + static_cast( + MatchedDecl->getDeclContext() )->getTemplateSpecializationKind(); + if( tsk == TSK_ImplicitInstantiation ) { + // This happens for e.g. a parameter 'T a' to an instantiated + // template function where T is long. We don't want to report such + // cases. + return; + } + } Check.diag( MatchedDecl->getLocation(), "Function %0 declared as returning %1. %2." ) << MatchedDecl << Type << alternatives; diff --git a/tools/clang-tidy-plugin/test/no-long.cpp b/tools/clang-tidy-plugin/test/no-long.cpp index 48d9634f7a50d..f80246f9a35b2 100644 --- a/tools/clang-tidy-plugin/test/no-long.cpp +++ b/tools/clang-tidy-plugin/test/no-long.cpp @@ -53,3 +53,14 @@ void h() g( ULONG_MAX ); // CHECK-MESSAGES: warning: Use of long-specific macro ULONG_MAX [cata-no-long] } + +template +struct A { + T Af0(); + long Af1(); + // CHECK-MESSAGES: warning: Function 'Af1' declared as returning 'long'. Prefer int or int64_t to long. [cata-no-long] + T Af2( long Af2i ); + // CHECK-MESSAGES: warning: Variable 'Af2i' declared as 'long'. Prefer int or int64_t to long. [cata-no-long] +}; + +A a; From 037c2501ad3685249d4431578550ec7f719df27d Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Tue, 11 Jun 2019 20:56:43 +0100 Subject: [PATCH 21/28] Avoid false positives on deduced return types We were reporting problems with deduced long return types (e.g. on lambdas) where we shouldn't. --- tools/clang-tidy-plugin/NoLongCheck.cpp | 2 +- tools/clang-tidy-plugin/test/no-long.cpp | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/tools/clang-tidy-plugin/NoLongCheck.cpp b/tools/clang-tidy-plugin/NoLongCheck.cpp index 22f9728c4b529..c2eaf7e91d349 100644 --- a/tools/clang-tidy-plugin/NoLongCheck.cpp +++ b/tools/clang-tidy-plugin/NoLongCheck.cpp @@ -97,7 +97,7 @@ static void CheckReturn( NoLongCheck &Check, const MatchFinder::MatchResult &Res if( !MatchedDecl || !MatchedDecl->getLocation().isValid() ) { return; } - QualType Type = MatchedDecl->getReturnType(); + QualType Type = MatchedDecl->getDeclaredReturnType(); std::string alternatives = AlternativesFor( Type ); if( alternatives.empty() ) { return; diff --git a/tools/clang-tidy-plugin/test/no-long.cpp b/tools/clang-tidy-plugin/test/no-long.cpp index f80246f9a35b2..76b825cfac2ed 100644 --- a/tools/clang-tidy-plugin/test/no-long.cpp +++ b/tools/clang-tidy-plugin/test/no-long.cpp @@ -64,3 +64,8 @@ struct A { }; A a; + +auto l0 = []( int64_t a ) +{ + return a; +}; From a30b6fecc70f96e6235accedf75778616e762ba7 Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Wed, 12 Jun 2019 07:43:21 +0100 Subject: [PATCH 22/28] Change naming convention in NoLongCheck test --- tools/clang-tidy-plugin/test/no-long.cpp | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/tools/clang-tidy-plugin/test/no-long.cpp b/tools/clang-tidy-plugin/test/no-long.cpp index 76b825cfac2ed..7b7be0001b623 100644 --- a/tools/clang-tidy-plugin/test/no-long.cpp +++ b/tools/clang-tidy-plugin/test/no-long.cpp @@ -3,20 +3,20 @@ #include #include -long a1; -// CHECK-MESSAGES: warning: Variable 'a1' declared as 'long'. Prefer int or int64_t to long. [cata-no-long] +long i1; +// CHECK-MESSAGES: warning: Variable 'i1' declared as 'long'. Prefer int or int64_t to long. [cata-no-long] -unsigned long a2; -// CHECK-MESSAGES: warning: Variable 'a2' declared as 'unsigned long'. Prefer unsigned int or uint64_t to unsigned long. [cata-no-long] +unsigned long i2; +// CHECK-MESSAGES: warning: Variable 'i2' declared as 'unsigned long'. Prefer unsigned int or uint64_t to unsigned long. [cata-no-long] -const long a3 = 0; -// CHECK-MESSAGES: warning: Variable 'a3' declared as 'const long'. Prefer int or int64_t to long. [cata-no-long] +const long i3 = 0; +// CHECK-MESSAGES: warning: Variable 'i3' declared as 'const long'. Prefer int or int64_t to long. [cata-no-long] -long &a4 = a1; -// CHECK-MESSAGES: warning: Variable 'a4' declared as 'long &'. Prefer int or int64_t to long. [cata-no-long] +long &i4 = i1; +// CHECK-MESSAGES: warning: Variable 'i4' declared as 'long &'. Prefer int or int64_t to long. [cata-no-long] -int64_t c; -uint64_t d; +int64_t i5; +uint64_t i6; void f1( long e ); // CHECK-MESSAGES: warning: Variable 'e' declared as 'long'. Prefer int or int64_t to long. [cata-no-long] @@ -27,9 +27,9 @@ long f2(); int64_t f3(); auto f4() -> decltype( 0L ); -int i1 = static_cast( 0 ); +int c0 = static_cast( 0 ); // CHECK-MESSAGES: warning: Static cast to 'long'. Prefer int or int64_t to long. [cata-no-long] -int i2 = static_cast( 0 ); +int c1 = static_cast( 0 ); template void g( T gp0, long gp1 = 0 ) From 8fe1cdaa46220a177754bf2d1d4544fcd96d9857 Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Wed, 12 Jun 2019 07:48:57 +0100 Subject: [PATCH 23/28] Avoid false-positives on deduced variable types Auto types which happen to be long shouldn't trigger a warning. --- tools/clang-tidy-plugin/NoLongCheck.cpp | 3 ++- tools/clang-tidy-plugin/test/no-long.cpp | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/tools/clang-tidy-plugin/NoLongCheck.cpp b/tools/clang-tidy-plugin/NoLongCheck.cpp index c2eaf7e91d349..168af4330d3ca 100644 --- a/tools/clang-tidy-plugin/NoLongCheck.cpp +++ b/tools/clang-tidy-plugin/NoLongCheck.cpp @@ -40,7 +40,8 @@ void NoLongCheck::registerPPCallbacks( CompilerInstance &Compiler ) void NoLongCheck::registerMatchers( MatchFinder *Finder ) { using TypeMatcher = clang::ast_matchers::internal::Matcher; - const TypeMatcher isIntegerOrRef = anyOf( isInteger(), references( isInteger() ) ); + const TypeMatcher isIntegerOrRef = + qualType( anyOf( isInteger(), references( isInteger() ) ), unless( autoType() ) ); Finder->addMatcher( valueDecl( hasType( isIntegerOrRef ) ).bind( "decl" ), this ); Finder->addMatcher( functionDecl( returns( isIntegerOrRef ) ).bind( "return" ), this ); Finder->addMatcher( cxxStaticCastExpr( hasDestinationType( isIntegerOrRef ) ).bind( "cast" ), diff --git a/tools/clang-tidy-plugin/test/no-long.cpp b/tools/clang-tidy-plugin/test/no-long.cpp index 7b7be0001b623..b1d87ad941f44 100644 --- a/tools/clang-tidy-plugin/test/no-long.cpp +++ b/tools/clang-tidy-plugin/test/no-long.cpp @@ -18,6 +18,8 @@ long &i4 = i1; int64_t i5; uint64_t i6; +auto i7 = int64_t {}; + void f1( long e ); // CHECK-MESSAGES: warning: Variable 'e' declared as 'long'. Prefer int or int64_t to long. [cata-no-long] From cef5be2ed6e7c5cd574c1e629fa5a6aa5e35d00c Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Thu, 13 Jun 2019 23:10:38 +0100 Subject: [PATCH 24/28] cata-no-long: Avoid false-positives on auto& --- tools/clang-tidy-plugin/NoLongCheck.cpp | 3 ++- tools/clang-tidy-plugin/test/no-long.cpp | 12 +++++++++--- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/tools/clang-tidy-plugin/NoLongCheck.cpp b/tools/clang-tidy-plugin/NoLongCheck.cpp index 168af4330d3ca..a7e75d3998d34 100644 --- a/tools/clang-tidy-plugin/NoLongCheck.cpp +++ b/tools/clang-tidy-plugin/NoLongCheck.cpp @@ -41,7 +41,8 @@ void NoLongCheck::registerMatchers( MatchFinder *Finder ) { using TypeMatcher = clang::ast_matchers::internal::Matcher; const TypeMatcher isIntegerOrRef = - qualType( anyOf( isInteger(), references( isInteger() ) ), unless( autoType() ) ); + qualType( anyOf( isInteger(), references( isInteger() ) ), + unless( autoType() ), unless( references( autoType() ) ) ); Finder->addMatcher( valueDecl( hasType( isIntegerOrRef ) ).bind( "decl" ), this ); Finder->addMatcher( functionDecl( returns( isIntegerOrRef ) ).bind( "return" ), this ); Finder->addMatcher( cxxStaticCastExpr( hasDestinationType( isIntegerOrRef ) ).bind( "cast" ), diff --git a/tools/clang-tidy-plugin/test/no-long.cpp b/tools/clang-tidy-plugin/test/no-long.cpp index b1d87ad941f44..aa9dcf9832b4b 100644 --- a/tools/clang-tidy-plugin/test/no-long.cpp +++ b/tools/clang-tidy-plugin/test/no-long.cpp @@ -15,10 +15,16 @@ const long i3 = 0; long &i4 = i1; // CHECK-MESSAGES: warning: Variable 'i4' declared as 'long &'. Prefer int or int64_t to long. [cata-no-long] -int64_t i5; -uint64_t i6; +long &&i5 = 0L; +// CHECK-MESSAGES: warning: Variable 'i5' declared as 'long &&'. Prefer int or int64_t to long. [cata-no-long] -auto i7 = int64_t {}; +int64_t i6; +uint64_t i7; + +auto i8 = int64_t {}; +auto &i9 = i1; +const auto &i10 = i1; +//auto&& i11 = i1; // Shouldn't cause a warning but I can't fix it void f1( long e ); // CHECK-MESSAGES: warning: Variable 'e' declared as 'long'. Prefer int or int64_t to long. [cata-no-long] From 9cce0627ee42f09021943c3bf30ca65775d4135b Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Thu, 13 Jun 2019 23:59:29 +0100 Subject: [PATCH 25/28] Avoid false positive on autogenerated code The analysis runs on compiler-generated member functions. We need to ignore variables in those. --- tools/clang-tidy-plugin/NoLongCheck.cpp | 5 +++++ tools/clang-tidy-plugin/test/no-long.cpp | 19 +++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/tools/clang-tidy-plugin/NoLongCheck.cpp b/tools/clang-tidy-plugin/NoLongCheck.cpp index a7e75d3998d34..2a0320c90d474 100644 --- a/tools/clang-tidy-plugin/NoLongCheck.cpp +++ b/tools/clang-tidy-plugin/NoLongCheck.cpp @@ -74,6 +74,11 @@ static void CheckDecl( NoLongCheck &Check, const MatchFinder::MatchResult &Resul if( alternatives.empty() ) { return; } + if( MatchedDecl->getName().startswith( "__" ) ) { + // Can happen for e.g. compiler-generated code inside an implicitly + // generated function + return; + } Decl::Kind contextKind = MatchedDecl->getDeclContext()->getDeclKind(); if( contextKind == Decl::Function || contextKind == Decl::CXXMethod || contextKind == Decl::CXXConstructor || contextKind == Decl::CXXConversion || diff --git a/tools/clang-tidy-plugin/test/no-long.cpp b/tools/clang-tidy-plugin/test/no-long.cpp index aa9dcf9832b4b..4549f74a5d1f0 100644 --- a/tools/clang-tidy-plugin/test/no-long.cpp +++ b/tools/clang-tidy-plugin/test/no-long.cpp @@ -64,6 +64,11 @@ void h() template struct A { + A(); + A( const A & ); + A( A && ); + A &operator=( const A & ); + A &operator=( A && ); T Af0(); long Af1(); // CHECK-MESSAGES: warning: Function 'Af1' declared as returning 'long'. Prefer int or int64_t to long. [cata-no-long] @@ -77,3 +82,17 @@ auto l0 = []( int64_t a ) { return a; }; + +template +struct B { + A BA[size][size]; +}; + +void Bf() +{ + B<12> b0; + B<12> b1; + // This exercises an obscure corner case where a defaulted operator= will + // cause the compiler to generate code involving an unsigned long variable. + b1 = static_cast < B<12> && >( b0 ); +} From 63e5e1ed5dcc66ed38ee8fc91e48048e3ec0ddfe Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Fri, 14 Jun 2019 19:44:38 +0100 Subject: [PATCH 26/28] Avoid false-positives on templated return types When an instantiation of a function template returns long, we don't want to report that; if it was written as long it will have already been reported on the template. --- tools/clang-tidy-plugin/NoLongCheck.cpp | 3 +++ tools/clang-tidy-plugin/test/no-long.cpp | 14 +++++++++----- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/tools/clang-tidy-plugin/NoLongCheck.cpp b/tools/clang-tidy-plugin/NoLongCheck.cpp index 2a0320c90d474..bcfceb4ef0780 100644 --- a/tools/clang-tidy-plugin/NoLongCheck.cpp +++ b/tools/clang-tidy-plugin/NoLongCheck.cpp @@ -109,6 +109,9 @@ static void CheckReturn( NoLongCheck &Check, const MatchFinder::MatchResult &Res if( alternatives.empty() ) { return; } + if( MatchedDecl->isTemplateInstantiation() ) { + return; + } Decl::Kind contextKind = MatchedDecl->getDeclContext()->getDeclKind(); if( contextKind == Decl::ClassTemplateSpecialization ) { diff --git a/tools/clang-tidy-plugin/test/no-long.cpp b/tools/clang-tidy-plugin/test/no-long.cpp index 4549f74a5d1f0..4741a5ecb5dbd 100644 --- a/tools/clang-tidy-plugin/test/no-long.cpp +++ b/tools/clang-tidy-plugin/test/no-long.cpp @@ -40,7 +40,7 @@ int c0 = static_cast( 0 ); int c1 = static_cast( 0 ); template -void g( T gp0, long gp1 = 0 ) +T g0( T gp0, long gp1 = 0 ) { // CHECK-MESSAGES: warning: Variable 'gp1' declared as 'long'. Prefer int or int64_t to long. [cata-no-long] long gi0; @@ -50,15 +50,15 @@ void g( T gp0, long gp1 = 0 ) void h() { - g( 0, 0 ); + g0( 0, 0 ); // Would like to report an error here for the template argument, but have // not found a way to do so. - g( LONG_MIN ); + g0( LONG_MIN ); // CHECK-MESSAGES: warning: Use of long-specific macro LONG_MIN [cata-no-long] - g( LONG_MAX ); + g0( LONG_MAX ); // CHECK-MESSAGES: warning: Use of long-specific macro LONG_MAX [cata-no-long] - g( ULONG_MAX ); + g0( ULONG_MAX ); // CHECK-MESSAGES: warning: Use of long-specific macro ULONG_MAX [cata-no-long] } @@ -96,3 +96,7 @@ void Bf() // cause the compiler to generate code involving an unsigned long variable. b1 = static_cast < B<12> && >( b0 ); } + +template +long g1( T g1p0 ); +// CHECK-MESSAGES: warning: Function 'g1' declared as returning 'long'. Prefer int or int64_t to long. [cata-no-long] From ac2ec5160a9e7b71016c507ba6be8821d7c0c9a7 Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Wed, 19 Jun 2019 08:16:27 +0100 Subject: [PATCH 27/28] Mention size_t as an alternative to unsigned int size_t is fairly often what people really want when using unsigned int. --- tools/clang-tidy-plugin/NoLongCheck.cpp | 2 +- tools/clang-tidy-plugin/test/no-long.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/clang-tidy-plugin/NoLongCheck.cpp b/tools/clang-tidy-plugin/NoLongCheck.cpp index bcfceb4ef0780..2113f98a57bdd 100644 --- a/tools/clang-tidy-plugin/NoLongCheck.cpp +++ b/tools/clang-tidy-plugin/NoLongCheck.cpp @@ -57,7 +57,7 @@ static std::string AlternativesFor( QualType Type ) if( name == "long" ) { return "Prefer int or int64_t to long"; } else if( name == "unsigned long" ) { - return "Prefer unsigned int or uint64_t to unsigned long"; + return "Prefer unsigned int, size_t, or uint64_t to unsigned long"; } else { return {}; } diff --git a/tools/clang-tidy-plugin/test/no-long.cpp b/tools/clang-tidy-plugin/test/no-long.cpp index 4741a5ecb5dbd..9ea4b1b0c9643 100644 --- a/tools/clang-tidy-plugin/test/no-long.cpp +++ b/tools/clang-tidy-plugin/test/no-long.cpp @@ -7,7 +7,7 @@ long i1; // CHECK-MESSAGES: warning: Variable 'i1' declared as 'long'. Prefer int or int64_t to long. [cata-no-long] unsigned long i2; -// CHECK-MESSAGES: warning: Variable 'i2' declared as 'unsigned long'. Prefer unsigned int or uint64_t to unsigned long. [cata-no-long] +// CHECK-MESSAGES: warning: Variable 'i2' declared as 'unsigned long'. Prefer unsigned int, size_t, or uint64_t to unsigned long. [cata-no-long] const long i3 = 0; // CHECK-MESSAGES: warning: Variable 'i3' declared as 'const long'. Prefer int or int64_t to long. [cata-no-long] From 2d801eaf233ca3ba8fddc52b5b7f2b5ce1a8810c Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Wed, 19 Jun 2019 08:17:25 +0100 Subject: [PATCH 28/28] Fix a couple of casts to unsigned long These had snuck in while I was getting all my other "removing long" PRs merged. --- tests/colony_test.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/colony_test.cpp b/tests/colony_test.cpp index 0d0bc9d698498..f3c8eb5f17220 100644 --- a/tests/colony_test.cpp +++ b/tests/colony_test.cpp @@ -328,7 +328,7 @@ TEST_CASE( "insert and erase", "[colony]" ) } while( count < 15000 ); // Erase randomly till half-empty - CHECK( test_colony.size() == static_cast( 30000 - count ) ); + CHECK( test_colony.size() == static_cast( 30000 - count ) ); for( int i = 0; i < count; ++i ) { test_colony.insert( 1 ); @@ -497,7 +497,7 @@ TEST_CASE( "insert and erase", "[colony]" ) } // Multiple sequential small insert/erase commands - CHECK( test_colony.size() == static_cast( count ) ); + CHECK( test_colony.size() == static_cast( count ) ); } TEST_CASE( "range erase", "[colony]" )