[PATCH] D123655: [clang-tidy] Add portability-std-allocator-const-t check

[Sam McCall via Phabricator via cfe-commits]

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