sammccall added a comment. This looks nice, my main substantive question is about matching VarDecl vs TypeLoc. Maybe there's a good reason to keep it this way but I'd like to understand why.
The rest is nits, feel free to ignore any bits you disagree with. ================ Comment at: clang-tools-extra/clang-tidy/portability/PortabilityTidyModule.cpp:28 + CheckFactories.registerCheck<StdAllocatorConstTCheck>( + "portability-std-allocator-const-t"); } ---------------- without the punctuation, the "t" AllocatorConstT/allocator-const-t doesn't seem so obvious IMO, I'd consider dropping it from the check name. Also the docs and even the implementation are focused on containers, "portability-const-container" might be easier to remember/understand for people not immersed in the details here. All up to you though. ================ Comment at: clang-tools-extra/clang-tidy/portability/StdAllocatorConstTCheck.cpp:25 + + // Match `std::vector<const T> var;` and other common containers like deque, + // list, and absl::flat_hash_set. Some containers like forward_list, queue, ---------------- If creating `allocator<const>` is always invalid, why be so careful here about which containers we're matching? ================ Comment at: clang-tools-extra/clang-tidy/portability/StdAllocatorConstTCheck.cpp:27 + // list, and absl::flat_hash_set. Some containers like forward_list, queue, + // and stack are not caught. Don't bother with std::unordered_set<const T> + // since libc++ does not allow it. ---------------- we can tell the "not caught" containers from the list below, but it'd be useful for the comment to say why ================ Comment at: clang-tools-extra/clang-tidy/portability/StdAllocatorConstTCheck.cpp:28 + // and stack are not caught. Don't bother with std::unordered_set<const T> + // since libc++ does not allow it. + Finder->addMatcher( ---------------- coupling this check to what libc++ currently supports makes it harder to understand, what's the cost of including unordered_set in this list anyway? ================ Comment at: clang-tools-extra/clang-tidy/portability/StdAllocatorConstTCheck.cpp:30 + Finder->addMatcher( + varDecl( + hasType(hasUnqualifiedDesugaredType( ---------------- Matching VarDecl specifically is an interesting choice. Maybe a good one: it does a good job of ensuring we diagnose at an appropriate place and only once. Other tempting choices are: - typeloc(loc(...)) - requires the type to be spelled somewhere, I think this can work even in template instantiations - expr(hasType(...)) - problem is it's likely to find multiple matches for the same root cause My guess is matching the TypeLoc would work better: catching some extra cases without much downside. ================ Comment at: clang-tools-extra/clang-tidy/portability/StdAllocatorConstTCheck.cpp:31 + varDecl( + hasType(hasUnqualifiedDesugaredType( + recordType(hasDeclaration(classTemplateSpecializationDecl( ---------------- Maybe you'd want to handle cases where the type is buried a bit, like `vector&` and `vector*` etc. Matching TypeLoc would take care of this for you, as matchers will see the nested TypeLoc. ================ Comment at: clang-tools-extra/clang-tidy/portability/StdAllocatorConstTCheck.cpp:50 + "container using std::allocator<const T> is " + "undefined; remove const to work with libstdc++ and future libc++"); +} ---------------- I'm a bit concerned that the second half: - is jargon that will confuse some people - excludes MSVC (are there others?) - will be inaccurate once libc++ changes Also nits: "undefined" might be confusingly close to "not defined" in the "no visible definition" sense, and "remove const to work with" reads a little oddly because it's a person that's removing but the code that's working. What about "container using std::allocator<const T> is invalid; remove const" or if focusing on the libc++ situation: "container using std::allocator<const T> is a libc++ extension; remove const for compatibility with other standard libraries"? ================ Comment at: clang-tools-extra/docs/clang-tidy/checks/portability-std-allocator-const-t.rst:6 + +Per C++ ``[allocator.requirements.general]``: "T is any cv-unqualified object +type", ``std::allocator<const T>`` is undefined. Many standard containers use ---------------- this seems a bit standard-ese for the intro paragraph. Maybe add a paragraph before it: "This check reports use of `vector<const T>` (and similar containers of const elements). These are not allowed in standard C++, and should usually be `vector<T>` instead." ================ Comment at: clang-tools-extra/docs/clang-tidy/checks/portability-std-allocator-const-t.rst:11 + +libstdc++ never supports ``std::allocator<const T>`` and containers using them. +Since GCC 8.0 (`PR48101 <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=48101>`), ---------------- It's a little confusing to call out only libstdc++ here without context when libc++ is the odd-one out. Maybe first mention that libc++ supports this as an extension that may be removed in the future? And if we're mentioning libstdc++'s behavior we should probably mention MS STL too which is similar to libstdc++: https://godbolt.org/z/c4o5nc66v ================ Comment at: clang-tools-extra/docs/clang-tidy/checks/portability-std-allocator-const-t.rst:21 + +However, code bases not compiled with libstdc++ may accrue such undefined usage. +This check finds such code and prevents backsliding while clean-up is ongoing. ---------------- again, I'd probably phrase this as "only compiled with libc++" Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123655/new/ https://reviews.llvm.org/D123655 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
