Author: Fangrui Song Date: 2022-04-13T22:35:11-07:00 New Revision: 73da7eed8fac84c9005518740f12d58389998d95
URL: https://github.com/llvm/llvm-project/commit/73da7eed8fac84c9005518740f12d58389998d95 DIFF: https://github.com/llvm/llvm-project/commit/73da7eed8fac84c9005518740f12d58389998d95.diff LOG: [clang-tidy] Add portability-std-allocator-const check Report use of ``std::vector<const T>`` (and similar containers of const elements). These are now allowed in standard C++ due to undefined ``std::allocator<const T>``. They do not compile with libstdc++ or MSVC. Future libc++ will remove the extension (D120996). See docs/clang-tidy/checks/portability-std-allocator-const.rst for detail. I have attempted clean-up in a large code base. Here are some statistics: * 98% are related to the container `std::vector`, among `deque/forward_list/list/multiset/queue/set/stack/vector`. * 24% are related to `std::vector<const std::string>`. * Both `std::vector<const absl::string_view>` and `std::vector<const int>` contribute 2%. The other contributors spread over various class types. The check can be useful to other large code bases and may serve as an example for future libc++ strictness improvement. Reviewed By: sammccall Differential Revision: https://reviews.llvm.org/D123655 Added: clang-tools-extra/clang-tidy/portability/StdAllocatorConstCheck.cpp clang-tools-extra/clang-tidy/portability/StdAllocatorConstCheck.h clang-tools-extra/docs/clang-tidy/checks/portability-std-allocator-const.rst clang-tools-extra/test/clang-tidy/checkers/portability-std-allocator-const.cpp Modified: clang-tools-extra/clang-tidy/portability/CMakeLists.txt clang-tools-extra/clang-tidy/portability/PortabilityTidyModule.cpp clang-tools-extra/docs/ReleaseNotes.rst clang-tools-extra/docs/clang-tidy/checks/list.rst Removed: ################################################################################ diff --git a/clang-tools-extra/clang-tidy/portability/CMakeLists.txt b/clang-tools-extra/clang-tidy/portability/CMakeLists.txt index a0de5871e036a..579e459bc72bc 100644 --- a/clang-tools-extra/clang-tidy/portability/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/portability/CMakeLists.txt @@ -7,6 +7,7 @@ add_clang_library(clangTidyPortabilityModule PortabilityTidyModule.cpp RestrictSystemIncludesCheck.cpp SIMDIntrinsicsCheck.cpp + StdAllocatorConstCheck.cpp LINK_LIBS clangTidy diff --git a/clang-tools-extra/clang-tidy/portability/PortabilityTidyModule.cpp b/clang-tools-extra/clang-tidy/portability/PortabilityTidyModule.cpp index c87a119aa81fc..1e4f1d5de1d97 100644 --- a/clang-tools-extra/clang-tidy/portability/PortabilityTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/portability/PortabilityTidyModule.cpp @@ -11,6 +11,7 @@ #include "../ClangTidyModuleRegistry.h" #include "RestrictSystemIncludesCheck.h" #include "SIMDIntrinsicsCheck.h" +#include "StdAllocatorConstCheck.h" namespace clang { namespace tidy { @@ -23,6 +24,8 @@ class PortabilityModule : public ClangTidyModule { "portability-restrict-system-includes"); CheckFactories.registerCheck<SIMDIntrinsicsCheck>( "portability-simd-intrinsics"); + CheckFactories.registerCheck<StdAllocatorConstCheck>( + "portability-std-allocator-const"); } }; diff --git a/clang-tools-extra/clang-tidy/portability/StdAllocatorConstCheck.cpp b/clang-tools-extra/clang-tidy/portability/StdAllocatorConstCheck.cpp new file mode 100644 index 0000000000000..a95048d71ef92 --- /dev/null +++ b/clang-tools-extra/clang-tidy/portability/StdAllocatorConstCheck.cpp @@ -0,0 +1,71 @@ +//===-- StdAllocatorConstCheck.cpp - clang-tidy --------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "StdAllocatorConstCheck.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace portability { + +void StdAllocatorConstCheck::registerMatchers(MatchFinder *Finder) { + // Match std::allocator<const T>. + auto allocatorConst = + recordType(hasDeclaration(classTemplateSpecializationDecl( + hasName("::std::allocator"), + hasTemplateArgument(0, refersToType(qualType(isConstQualified())))))); + + auto hasContainerName = + hasAnyName("::std::vector", "::std::deque", "::std::list", + "::std::multiset", "::std::set", "::std::unordered_multiset", + "::std::unordered_set", "::absl::flat_hash_set"); + + // Match `std::vector<const T> var;` and other common containers like deque, + // list, and absl::flat_hash_set. Containers like queue and stack use deque + // but do not directly use std::allocator as a template argument, so they + // aren't caught. + Finder->addMatcher( + typeLoc( + templateSpecializationTypeLoc(), + loc(hasUnqualifiedDesugaredType(anyOf( + recordType(hasDeclaration(classTemplateSpecializationDecl( + hasContainerName, + anyOf( + hasTemplateArgument(1, refersToType(allocatorConst)), + hasTemplateArgument(2, refersToType(allocatorConst)), + hasTemplateArgument(3, refersToType(allocatorConst)))))), + // Match std::vector<const dependent> + templateSpecializationType( + templateArgumentCountIs(1), + hasTemplateArgument( + 0, refersToType(qualType(isConstQualified()))), + hasDeclaration(namedDecl(hasContainerName))))))) + .bind("type_loc"), + this); +} + +void StdAllocatorConstCheck::check(const MatchFinder::MatchResult &Result) { + const auto *T = Result.Nodes.getNodeAs<TypeLoc>("type_loc"); + if (!T) + return; + // Exclude TypeLoc matches in STL headers. + if (isSystem(Result.Context->getSourceManager().getFileCharacteristic( + T->getBeginLoc()))) + return; + + diag(T->getBeginLoc(), + "container using std::allocator<const T> is a deprecated libc++ " + "extension; remove const for compatibility with other standard " + "libraries"); +} + +} // namespace portability +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/portability/StdAllocatorConstCheck.h b/clang-tools-extra/clang-tidy/portability/StdAllocatorConstCheck.h new file mode 100644 index 0000000000000..98a7b473e17e1 --- /dev/null +++ b/clang-tools-extra/clang-tidy/portability/StdAllocatorConstCheck.h @@ -0,0 +1,37 @@ +//===--- StdAllocatorConstT.h - clang-tidy -----------------------*- C++-*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PORTABILITY_STDALLOCATORCONSTCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PORTABILITY_STDALLOCATORCONSTCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang { +namespace tidy { +namespace portability { + +/// Report use of ``std::vector<const T>`` (and similar containers of const +/// elements). These are not allowed in standard C++ due to undefined +/// ``std::allocator<const T>``. They do not compile with libstdc++ or MSVC. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/portability-std-allocator-const.html +class StdAllocatorConstCheck : public ClangTidyCheck { +public: + StdAllocatorConstCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace portability +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PORTABILITY_STDALLOCATORCONSTCHECK_H diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 870a043c6b813..5392ab5758f59 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -117,6 +117,14 @@ New checks Replaces groups of adjacent macros with an unscoped anonymous enum. +- New :doc:`portability-std-allocator-const <clang-tidy/checks/portability-std-allocator-const>` check. + + Report use of ``std::vector<const T>`` (and similar containers of const + elements). These are not allowed in standard C++ due to undefined + ``std::allocator<const T>``. They do not compile with libstdc++ or MSVC. + Future libc++ will remove the extension (`D120996 + <https://reviews.llvm.org/D120996>`). + New check aliases ^^^^^^^^^^^^^^^^^ diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index e9367216ac515..6b3a6d83a361c 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -289,6 +289,7 @@ Clang-Tidy Checks `performance-unnecessary-value-param <performance-unnecessary-value-param.html>`_, "Yes" `portability-restrict-system-includes <portability-restrict-system-includes.html>`_, "Yes" `portability-simd-intrinsics <portability-simd-intrinsics.html>`_, + `portability-std-allocator-const <portability-std-allocator-const.html>`_, `readability-avoid-const-params-in-decls <readability-avoid-const-params-in-decls.html>`_, "Yes" `readability-braces-around-statements <readability-braces-around-statements.html>`_, "Yes" `readability-const-return-type <readability-const-return-type.html>`_, "Yes" diff --git a/clang-tools-extra/docs/clang-tidy/checks/portability-std-allocator-const.rst b/clang-tools-extra/docs/clang-tidy/checks/portability-std-allocator-const.rst new file mode 100644 index 0000000000000..31463c2b65dd3 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/portability-std-allocator-const.rst @@ -0,0 +1,31 @@ +.. title:: clang-tidy - portability-std-allocator-const + +portability-std-allocator-const +=============================== + +Report use of ``std::vector<const T>`` (and similar containers of const +elements). These are not allowed in standard C++, and should usually be +``std::vector<T>`` instead." + +Per C++ ``[allocator.requirements.general]``: "T is any cv-unqualified object +type", ``std::allocator<const T>`` is undefined. Many standard containers use +``std::allocator`` by default and therefore their ``const T`` instantiations are +undefined. + +libc++ defines ``std::allocator<const T>`` as an extension which will be removed +in the future. + +libstdc++ and MSVC do not support ``std::allocator<const T>``: + +.. code:: c++ + + // libstdc++ has a better diagnostic since https://gcc.gnu.org/bugzilla/show_bug.cgi?id=48101 + std::deque<const int> deque; // error: static assertion failed: std::deque must have a non-const, non-volatile value_type + std::set<const int> set; // error: static assertion failed: std::set must have a non-const, non-volatile value_type + std::vector<int* const> vector; // error: static assertion failed: std::vector must have a non-const, non-volatile value_type + + // MSVC + // error C2338: static_assert failed: 'The C++ Standard forbids containers of const elements because allocator<const T> is ill-formed.' + +Code bases only compiled with libc++ may accrue such undefined usage. This +check finds such code and prevents backsliding while clean-up is ongoing. diff --git a/clang-tools-extra/test/clang-tidy/checkers/portability-std-allocator-const.cpp b/clang-tools-extra/test/clang-tidy/checkers/portability-std-allocator-const.cpp new file mode 100644 index 0000000000000..e6155592f33bc --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/portability-std-allocator-const.cpp @@ -0,0 +1,94 @@ +// RUN: %check_clang_tidy -std=c++11-or-later %s portability-std-allocator-const %t -- + +namespace std { +typedef unsigned size_t; + +template <class T> +class allocator {}; +template <class T> +class hash {}; +template <class T> +class equal_to {}; +template <class T> +class less {}; + +template <class T, class A = std::allocator<T>> +class deque {}; +template <class T, class A = std::allocator<T>> +class forward_list {}; +template <class T, class A = std::allocator<T>> +class list {}; +template <class T, class A = std::allocator<T>> +class vector {}; + +template <class K, class C = std::less<K>, class A = std::allocator<K>> +class multiset {}; +template <class K, class C = std::less<K>, class A = std::allocator<K>> +class set {}; +template <class K, class H = std::hash<K>, class Eq = std::equal_to<K>, class A = std::allocator<K>> +class unordered_multiset {}; +template <class K, class H = std::hash<K>, class Eq = std::equal_to<K>, class A = std::allocator<K>> +class unordered_set {}; + +template <class T, class C = std::deque<T>> +class stack {}; +} // namespace std + +namespace absl { +template <class K, class H = std::hash<K>, class Eq = std::equal_to<K>, class A = std::allocator<K>> +class flat_hash_set {}; +} // namespace absl + +template <class T> +class allocator {}; + +void simple(const std::vector<const char> &v, std::deque<const short> *d) { + // CHECK-MESSAGES: [[#@LINE-1]]:24: warning: container using std::allocator<const T> is a deprecated libc++ extension; remove const for compatibility with other standard libraries + // CHECK-MESSAGES: [[#@LINE-2]]:52: warning: container + std::list<const long> l; + // CHECK-MESSAGES: [[#@LINE-1]]:8: warning: container + + std::multiset<int *const> ms; + // CHECK-MESSAGES: [[#@LINE-1]]:8: warning: container + std::set<const std::hash<int>> s; + // CHECK-MESSAGES: [[#@LINE-1]]:8: warning: container + std::unordered_multiset<int *const> ums; + // CHECK-MESSAGES: [[#@LINE-1]]:8: warning: container + std::unordered_set<const int> us; + // CHECK-MESSAGES: [[#@LINE-1]]:8: warning: container + + absl::flat_hash_set<const int> fhs; + // CHECK-MESSAGES: [[#@LINE-1]]:9: warning: container + + using my_vector = std::vector<const int>; + // CHECK-MESSAGES: [[#@LINE-1]]:26: warning: container + my_vector v1; + using my_vector2 = my_vector; + + std::vector<int> neg1; + std::vector<const int *> neg2; // not const T + std::vector<const int, allocator<const int>> neg3; // not use std::allocator<const T> + std::allocator<const int> a; // not caught, but rare + std::forward_list<const int> forward; // not caught, but rare + std::stack<const int> stack; // not caught, but rare +} + +template <class T> +void temp1() { + std::vector<const T> v; + // CHECK-MESSAGES: [[#@LINE-1]]:8: warning: container + + std::vector<T> neg1; + std::forward_list<const T> neg2; +} +void use_temp1() { temp1<int>(); } + +template <class T> +void temp2() { + // Match std::vector<const dependent> for the uninstantiated temp2. + std::vector<const T> v; + // CHECK-MESSAGES: [[#@LINE-1]]:8: warning: container + + std::vector<T> neg1; + std::forward_list<const T> neg2; +} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
