JonasToth marked 3 inline comments as done.
JonasToth added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt:33
LINK_LIBS
+ clangAnalysis
clangTidy
----------------
sammccall wrote:
> 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.
i verified that the check works with clangd without any additional build
settings.
================
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:
> 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).
it is common to write custom matchers in clang-tidy first and use the internal
matcher apis as well in clang-tidy.
```
$ cd llvm-project/clang-tools-extra/clang-tidy
$ git grep "internal::" | wc -l
86
```
Usually, they are extracted once they are generally useful. And many checks
introduce custom matchers for better readability.
I can extract this matcher later if you want, but right now with the longevity
of the patch and it being common practice in clang-tidy I am opposed to it.
================
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);
----------------
JonasToth wrote:
> sammccall wrote:
> > 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).
> it is common to write custom matchers in clang-tidy first and use the
> internal matcher apis as well in clang-tidy.
>
> ```
> $ cd llvm-project/clang-tools-extra/clang-tidy
> $ git grep "internal::" | wc -l
> 86
> ```
>
> Usually, they are extracted once they are generally useful. And many checks
> introduce custom matchers for better readability.
> I can extract this matcher later if you want, but right now with the
> longevity of the patch and it being common practice in clang-tidy I am
> opposed to it.
thank you for that hint! that would explain the issues indeed. I try to verify
your proposed fix by re-transforming LLVM.
some other subscribers checked their own codebases as well. this would give us
more confidence too.
if the crash is still there, I would like to keep the 'safety-if' in `check`
and try to find the issue with the matcher.
The path @njames93 linked seemed like it would help there a lot.
================
Comment at:
clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:84
+ compoundStmt(
+ findAll(declStmt(allOf(containsDeclaration2(
+ LocalValDecl.bind("local-value")),
----------------
njames93 wrote:
> `allOf` is unnecessary here as `declStmt` is implicitly an `allOf` matcher.
> Also `findAll` isn't the right matcher in this case, you likely want
> `forEachDescendant`. `findAll` will try and match on the `CompoundStmt` which
> is always going to fail.
switching to `forEachDescendant`, both seem to work.
I am unsure why I used `findAll`, but I know that I started out with
`forEachDescendant` for some bits of the check and it was insufficent.
But it could very well be, that these pieces are now in the `ExprMutAnalyzer`.
================
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;
----------------
njames93 wrote:
> sammccall wrote:
> > 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.
> Ahh, hopefully once D118520 lands it should be easier to track down these
> bugs and create reproducers
I am rerunning the whole analysis on LLVM to see if the crash is still there
after the proposed fix of sam (thanks!).
I will remove the code completely if the crash is gone. Otherwise I'd rather
keep the safety in and try to fix the underlying issue afterwards.
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
