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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
