aaron.ballman added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp:316-317
return nullptr;
+ if (!Member->getMemberDecl()->getDeclName().isIdentifier())
+ return nullptr;
StringRef Name = Member->getMemberDecl()->getName();
----------------
njames93 wrote:
> njames93 wrote:
> > aaron.ballman wrote:
> > > It's really strange to me that we're even getting to this point in the
> > > check -- the only way for the assertion to fail is for the member call
> > > expression to be on something without a name. The cases I can think of
> > > for that would be something like `foo.operator+(RHS)` or something
> > > similarly nonsensical within this context (we're looking for things named
> > > `begin` or `end`). I think it'd make more sense to handle this at the
> > > matcher level (or early in the call chain) so that we never get here.
> > >
> > > I think having a test case would be really useful to trying to understand
> > > what changes are appropriate. I don't think these changes are wrong so
> > > much as I wonder if we're in the wrong place to make them (and we'll hit
> > > other confused code elsewhere).
> > It is very strange. At the matcher level we are looking for calls to
> > methods named `begin` or `end` or (c/r) variants.
> > I'm gonna try and put some debug prints and see if I can figure out the
> > code actually causing the assert in the first place.
> >
> > In the mean time, I have a patch in the works that moves a lot of the logic
> > into the matchers and this whole function is removed in there, however a
> > few creases still need to be ironed out in there.
> So I put some debugprints in there and managed to find the cause of the crash
> ```
> // llvm/lib/Option/OptTable.cpp:150
> for (StringSet<>::const_iterator I = PrefixesUnion.begin(),
> E = PrefixesUnion.end(); I != E; ++I) {
> StringRef Prefix = I->getKey();
> for (StringRef::const_iterator C = Prefix.begin(), CE = Prefix.end();
> C != CE; ++C)
> if (!is_contained(PrefixChars, *C))
> PrefixChars.push_back(*C);
> }```
> However I'm still not 100% sure of the cause of the crash. Its nothing to do
> with `begin()` being a member of the bast class StringMap. I tried testing
> with inherited begin/end methods and no crash.
> My best guess is Prefix.begin() returns `StringMapIterator<>`, that class
> contains a conversion operator to `StringMapConstIterator<>`. Conversion
> operators names aren't Identifiers so that would explain the assert.
> If that is the case, the actual issue is in `digThroughConstructors` being
> fooled by the `CXXMemberCallExpr` that's just a conversion operator.
>
> The other patch in the works doesn't use that function so the bug is not
> present there, maybe its easier to just get that ready. WDYT?
> The other patch in the works doesn't use that function so the bug is not
> present there, maybe its easier to just get that ready. WDYT?
I think that makes sense.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D97889/new/
https://reviews.llvm.org/D97889
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
