steveire marked an inline comment as done.
steveire added inline comments.

Comment at: lib/ASTMatchers/ASTMatchersInternal.cpp:240
+  assert(RestrictKind.isBaseOf(NodeKind));
+  if (Implementation->dynMatches(N, Finder, Builder)) {
aaron.ballman wrote:
> steveire wrote:
> > aaron.ballman wrote:
> > > Add an assertion message?
> > Saying what? The original code doesn't have one. Let's avoid round trips in 
> > review comments :).
> This isn't a "round trip"; it's not unreasonable to ask people to NFC improve 
> the code they're touching (it's akin to saying "Because you figured out this 
> complex piece of code does X, can you add a comment to it so others don't 
> have to do that work next time.").
> As best I can tell, this assertion exists because this function is meant to 
> mirror `matches()` without this base check in release mode. You've lost that 
> mirroring with your refactoring, which looks suspicious. Is there a reason 
> this function deviates from `matches()` now?
> As for the assertion message itself, how about "matched a node of an 
> unexpected derived kind"?
Hmm, maybe that was a misunderstanding. Your note about an assertion message 
was not clear, so I asked you what you suggest the message should be. No need 
for offense :)

  rC Clang


cfe-commits mailing list

Reply via email to