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

[Aaron Ballman via Phabricator via cfe-commits]

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();
----------------
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).


================
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();
----------------
This doesn't seem necessary? Calling `NamedDecl::getName()` already asserts that the name is an identifier.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97889/new/

https://reviews.llvm.org/D97889