njames93 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();
----------------
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.
================
Comment at: clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp:549
const auto *AliasVar = cast<VarDecl>(AliasDecl->getSingleDecl());
+ assert(AliasVar->getDeclName().isIdentifier());
VarName = AliasVar->getName().str();
----------------
aaron.ballman wrote:
> This doesn't seem necessary? Calling `NamedDecl::getName()` already asserts
> that the name is an identifier.
I put that in there to figure out which `getName` assertion was firing as the
call stack was optimized away on RelWithAssert builds. Probably can be removed.
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
