[clang-tidy] problem finding AST matcher for nested implicit const-cast

Clang Frontend
1

Hi all,

I'm trying to fix the clang-tidy bug 35082 [1] as a starter for getting into LLVM development and contribution.

I think, I've tracked down the issue to

clang-tidy/modernize/LoopConvertCheck.cpp:123ff: clang::tidy::modernize::makeIteratorLoopMatcher()

  DeclarationMatcher InitDeclMatcher =
      varDecl(hasInitializer(anyOf(ignoringParenImpCasts(BeginCallMatcher),
                                   materializeTemporaryExpr(
                                       ignoringParenImpCasts(BeginCallMatcher)),
                                   hasDescendant(BeginCallMatcher))))
          .bind(InitVarName);

The definition of `InitDeclMatcher` seems to not match the AST tree

  >-ForStmt
  > >-DeclStmt
  > > `-VarDecl
  > >   `-ExprWithCleanups
  > >     `-ImplicitCastExpr
  > >       `-CXXConstructExpr
  > >         `-MaterializeTemporaryExpr
  > >           `-ImplicitCastExpr
  > >             `-CXXMemberCallExpr
  > >               `-MemberExpr 
  > >                 `-DeclRefExpr

of

std::vector<int> vec{1, 2, 3, 4, 5};
for(std::vector<int>::const_iterator i = vec.begin(); i != vec.end(); ++i) { ... }

In my naivety, I tried to include the following into the `anyOf`:

                                   exprWithCleanups(
                                       ignoringParenImpCasts(
                                           cxxConstructExpr(
                                               materializeTemporaryExpr(
                                                   ignoringParenImpCasts(
                                                       BeginCallMatcher))))),

mimicking the specific AST tree. Without luck.

I browsed through include/clang/ASTMatchers.h for better suited matchers, but couldn't find any.

What important detail am I missing?

Best
Torbjörn

[1]: https://bugs.llvm.org/show_bug.cgi?id=35082

2

The original matcher looks pretty weird to me, as the last anyOf() case, namely "hasDescendant(BeginCallMatcher)", already matches more than all previous anyOf() cases.

3

Hi Torbjörn:

This behavior appears to be intentional, though it would be friendly to emit a warning. I believe this is the case your are hitting:

794 } else if (!Context->hasSameType(CanonicalInitVarType,
795 CanonicalBeginType)) {
796 // Check for qualified types to avoid conversions from non-const to const
797 // iterator types.
798 return false;
799 }

hth…
don

4

I think you might be able to safely remove this if else, or at least limit it.

Consider the following:

// this is your test case. it’s legal, but currently ignored
std::vector v;
for(std::vector::const_iterator i = v.begin(); i != v.end(); ++i) {}

// these aren’t illegal, and therefore never even considered by the checker – they cause a compiler error
for(std::vector::iterator i = v.cbegin(); i != v.cend(); ++i) {}
const std::vector cv;
for(std::vector::iterator i = cv.begin(); i != cv.end(); ++i) {}

5

Hi Don,

thank you very much for your advice.

After playing around a little time with this I finally went with your
suggestion removing this branch altogether. No tests seem to fail.

I opened D61827 with the change.

Best
Torbjörn