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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay marked an inline comment as done.
MaskRay added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/portability/PortabilityTidyModule.cpp:28
+    CheckFactories.registerCheck<StdAllocatorConstTCheck>(
+        "portability-std-allocator-const-t");
   }
----------------
sammccall wrote:
> 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.
Renamed to portability-std-allocator-const. The intention is to catch std::allocator. If the check gets improved to catch more cases, we won't need to rename it from std-container-const to std-allocator-const :)


================
Comment at: clang-tools-extra/clang-tidy/portability/StdAllocatorConstTCheck.cpp:30
+  Finder->addMatcher(
+      varDecl(
+          hasType(hasUnqualifiedDesugaredType(
----------------
sammccall wrote:
> 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.
Thanks for Sam's offline help. I've got a form which is able to match many cases:)


================
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++");
+}
----------------
sammccall wrote:
> 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"?
Thanks for the suggestion! Adopted the libc++ oriented diagnostic.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123655/new/

https://reviews.llvm.org/D123655