Page MenuHomePhabricator

Extract method to allow re-use
AcceptedPublic

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

Details

Reviewers
aaron.ballman
Summary

The list of matchers which can be used with a top matcher can be used in
other contexts than code completion. It can be output as data in
clang-tidy.

Diff Detail

Event Timeline

steveire created this revision.Nov 11 2018, 2:31 PM

Generally LGTM but this has no test coverage. What are your plans for how you want to see this proceed? Do you intend to commit everything in one batch once all of the reviews are accepted, or do piecemeal commits?

lib/ASTMatchers/Dynamic/Registry.cpp
565

I'd prefer T to be named Callable and func to be Func. Also, I think it should be Callable &&Func.

596–639

Then you can use std::forward<Callable>(Func)(...); because we can't use std::invoke() yet.

I think this commit is fine without tests. The new method remains internal to the file but follow-up commits add public interface and tests.

I'll push the commits separately. I'll not be squashing them.

I think this commit is fine without tests.

That's not something we do except under specific extenuating circumstances (NFC fixes or changes for which existing coverage suffices), per our developer policy.

The new method remains internal to the file but follow-up commits add public interface and tests.

I'll push the commits separately. I'll not be squashing them.

I'd prefer to squash them at least into commits that have tests for the functionality. More specifically: if the functionality is testable, it needs tests when it's committed. If it's not testable, squash it into the most closely-related commit that is testable. In terms of review cycles, I'm fine with "this functionality will be commit with that functionality, whose tests cover this.", though I'd personally prefer future reviews to come pre-squashed to cut down on confusion.

This is just a NFC change, which is normal to appear without tests. The consensus on IRC is that this is fine.

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

This is just a NFC change, which is normal to appear without tests. The consensus on IRC is that this is fine.

This NFC would likely be rejected were it not for your other patches because it would serve no purpose while adding complexity and overhead. I'm not certain why you feel so strongly about not merging this with other patches. We ask developers to do that for new features because it makes it trivial for us to know what to revert when a random bot goes red. That said, if you continue to feel strongly about this, I'll hold my nose.

LGTM with some formatting nits.

lib/ASTMatchers/Dynamic/Registry.cpp
565

80 col.

595–639

80 col and elide braces.

This revision is now accepted and ready to land.Nov 12 2018, 3:52 PM