Page MenuHomePhabricator

Record the matcher type when storing a binding
AcceptedPublic

Authored by steveire on Nov 11 2018, 2:38 PM.

Details

Summary

This is necessary so that when we wish to print the matchers for a
binding of type CXXMeethodDecl, but which was matched with a base
matcher such as functionDecl() we can inform the user that they can
write functionDecl(cxxMethodDecl(isOverride())) etc.

Diff Detail

Event Timeline

steveire created this revision.Nov 11 2018, 2:38 PM
aaron.ballman added a subscriber: sbenza.

Adding @sbenza in case he has opinions on this approach. I think it's reasonable, but I also know that changes to the the AST matcher internals sometimes have unintended side effects with the various instantiations.

include/clang/ASTMatchers/ASTMatchersInternal.h
198–199

Use of a std::pair here is unfortunate because it's really hard to keep track of what first and second actually mean. A tiny aggregate with two named members would help.

Adding @sbenza in case he has opinions on this approach. I think it's reasonable, but I also know that changes to the the AST matcher internals sometimes have unintended side effects with the various instantiations.

The problem used to come with the number of template instantiations, but afaik this was with an MSVC configuration that is no longer supported.
In any case, I don't see this change making new template instantiations.

I also don't see any actual use of this newly stored data so I don't see the goal. Is this for diagnostics on failures or something like that?

Adding @sbenza in case he has opinions on this approach. I think it's reasonable, but I also know that changes to the the AST matcher internals sometimes have unintended side effects with the various instantiations.

The problem used to come with the number of template instantiations, but afaik this was with an MSVC configuration that is no longer supported.
In any case, I don't see this change making new template instantiations.

I also don't see any actual use of this newly stored data so I don't see the goal. Is this for diagnostics on failures or something like that?

This commit is part of a series. You can see the use of this in the next commit: https://reviews.llvm.org/D54408

Adding @sbenza in case he has opinions on this approach. I think it's reasonable, but I also know that changes to the the AST matcher internals sometimes have unintended side effects with the various instantiations.

The problem used to come with the number of template instantiations, but afaik this was with an MSVC configuration that is no longer supported.
In any case, I don't see this change making new template instantiations.

Ah, I think I was remembering the issues from binary sizes due to template instantiations. I didn't think this would introduce new instantiations, but I thought it might make them all larger in a harmful way. I don't think that's a concern here, but your second set of eyes doesn't hurt.

include/clang/ASTMatchers/ASTMatchersInternal.h
153

other -> Other

154

This doesn't provide the right ordering guarantees. Use std::tie() instead: return std::tie(DynNode, NodeKind) < std::tie(Other.DynNode, Other.NodeKind);

aaron.ballman accepted this revision.Nov 12 2018, 3:32 PM

Aside from a minor formatting nit, LGTM.

include/clang/ASTMatchers/ASTMatchersInternal.h
154

80 col.

This revision is now accepted and ready to land.Nov 12 2018, 3:32 PM
steveire added inline comments.Nov 12 2018, 3:35 PM
include/clang/ASTMatchers/ASTMatchersInternal.h
154

Thanks, I forgot about tie :).