[PATCH] D97889: [clang-tidy] Fix assert in modernize-loop-convert

[Nathan James via Phabricator via cfe-commits]

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