sammccall added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt:33
LINK_LIBS
+ clangAnalysis
clangTidy
----------------
JonasToth wrote:
> njames93 wrote:
> > Will this work under clangd as I don't think that links in the
> > clangAnalysis libraries @sammccall?
> I did not check if it works and `clangd` does not explicitly link to
> `Analysis`, but only to the checks.
> Doesn't the `LINK_LIBS` induce transitive dependency resolution for linking?
I don't know whether you need to add it everywhere, I suspect transitive is OK.
In any case this isn't a new dependency, Sema depends on Analysis and clangd
depends on Sema.
================
Comment at:
clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:29
+ ast_matchers::internal::Matcher<Decl>, InnerMatcher) {
+ return ast_matchers::internal::matchesFirstInPointerRange(
+ InnerMatcher, Node.decl_begin(), Node.decl_end(), Finder, Builder);
----------------
This returns an iterator (i.e. a pointer), which is being converted to a
boolean.
i.e. it's always returning true. I think this is why you're seeing nullptr
crashes on Variable.
================
Comment at:
clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:29
+ ast_matchers::internal::Matcher<Decl>, InnerMatcher) {
+ return ast_matchers::internal::matchesFirstInPointerRange(
+ InnerMatcher, Node.decl_begin(), Node.decl_end(), Finder, Builder);
----------------
sammccall wrote:
> This returns an iterator (i.e. a pointer), which is being converted to a
> boolean.
>
> i.e. it's always returning true. I think this is why you're seeing nullptr
> crashes on Variable.
this is depending on `::internal` details, which doesn't seem OK.
I think you'd need to find another way to do it, or move this to ASTMatchers
(in a separate patch).
================
Comment at:
clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:102-105
+ // There have been crashes on 'Variable == nullptr', even though the matcher
+ // is not conditional. This comes probably from 'findAll'-matching.
+ if (!Variable || !LocalScope)
+ return;
----------------
JonasToth wrote:
> njames93 wrote:
> > This sounds like a bug that should be raised with ASTMatchers, if its
> > reproducible.
> I found no way to reproduce it (yet).
> The crashes occured during verification on big projects and `run-clang-tidy`
> kind of obfuscated where the crash happened.
>
> This is something for the iron out part of the check I think.
This is a bug in a helper added in this patch, I've added a comment above.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54943/new/
https://reviews.llvm.org/D54943
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
