[PATCH] D54404: Exclude matchers which can have multiple results
aaron.ballman added inline comments. Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:624 + "hasAnyDeclaration", + "hasAnyName", + "hasAnyParameter", steveire wrote: > aaron.ballman wrote: > > steveire wrote: > > > sbenza wrote: > > > > I'm not sure what goes in this list. > > > > `hasAnyName` is here but not `hasName`. > > > > What is ambiguous about `hasAnyName`? > > > I have a follow-up which adds output showing that `hasName` can be used. > > > See > > > > > > http://ec2-52-14-16-249.us-east-2.compute.amazonaws.com:10240/z/_X9mnw > > > > > > If there was an entry there for `hasAnyName`, what would go in it? > > > If there was an entry there for hasAnyName, what would go in it? > > > > Presumably the same as `hasName()`, though for the purposes of that list, I > > could see why it would be a bit odd to list it. > > > > It almost feels like this isn't about ambiguity of the matchers (at least, > > not always) so much as it is about sensibility within a "the following are > > related matchers" list due to there being many different ways for matchers > > to relate. For instance, a related matcher could be > > `functionDecl(hasAnyParameter(anything()))`, but you might not want to list > > that because it's an open-ended problem to generate all such cases and it > > has very limited value to list them. Is that a better way for me to think > > about this? > > it is about sensibility ... Is that a better way for me to think about > > this? > > Yes. The list attempts to exclude things that don't make sense to show for > `matcher` output. Okay, so this isn't really about ambiguity as I was thinking of it (I was thinking of it in terms of "calling this matcher with this argument would be ambiguous"). Perhaps a comment like: ``` When matcher output is enabled, a list of related matchers is displayed to the user suggesting other matchers that would also be true for the matched construct. This is a list of matchers to exclude when considering those related matchers. Add elements to this list when the relationship to other matchers is impossible to determine or expected to be uninteresting to the user. For instance, when matching functionDecl() to a function declaration with a single parameter, it would not be interesting for the user to know that functionDecl(hasParameter(anything())) also matches. Given that the anything() matcher is unlikely to be of interest in any matcher relationship, it is listed below as one of the matchers to exclude. ``` (Feel free to reword, come up with more compelling examples, etc.) Repository: rC Clang https://reviews.llvm.org/D54404 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54404: Exclude matchers which can have multiple results
steveire added inline comments. Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:624 + "hasAnyDeclaration", + "hasAnyName", + "hasAnyParameter", aaron.ballman wrote: > steveire wrote: > > sbenza wrote: > > > I'm not sure what goes in this list. > > > `hasAnyName` is here but not `hasName`. > > > What is ambiguous about `hasAnyName`? > > I have a follow-up which adds output showing that `hasName` can be used. See > > > > http://ec2-52-14-16-249.us-east-2.compute.amazonaws.com:10240/z/_X9mnw > > > > If there was an entry there for `hasAnyName`, what would go in it? > > If there was an entry there for hasAnyName, what would go in it? > > Presumably the same as `hasName()`, though for the purposes of that list, I > could see why it would be a bit odd to list it. > > It almost feels like this isn't about ambiguity of the matchers (at least, > not always) so much as it is about sensibility within a "the following are > related matchers" list due to there being many different ways for matchers to > relate. For instance, a related matcher could be > `functionDecl(hasAnyParameter(anything()))`, but you might not want to list > that because it's an open-ended problem to generate all such cases and it has > very limited value to list them. Is that a better way for me to think about > this? > it is about sensibility ... Is that a better way for me to think about this? Yes. The list attempts to exclude things that don't make sense to show for `matcher` output. Repository: rC Clang https://reviews.llvm.org/D54404 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54404: Exclude matchers which can have multiple results
aaron.ballman added inline comments. Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:624 + "hasAnyDeclaration", + "hasAnyName", + "hasAnyParameter", steveire wrote: > sbenza wrote: > > I'm not sure what goes in this list. > > `hasAnyName` is here but not `hasName`. > > What is ambiguous about `hasAnyName`? > I have a follow-up which adds output showing that `hasName` can be used. See > > http://ec2-52-14-16-249.us-east-2.compute.amazonaws.com:10240/z/_X9mnw > > If there was an entry there for `hasAnyName`, what would go in it? > If there was an entry there for hasAnyName, what would go in it? Presumably the same as `hasName()`, though for the purposes of that list, I could see why it would be a bit odd to list it. It almost feels like this isn't about ambiguity of the matchers (at least, not always) so much as it is about sensibility within a "the following are related matchers" list due to there being many different ways for matchers to relate. For instance, a related matcher could be `functionDecl(hasAnyParameter(anything()))`, but you might not want to list that because it's an open-ended problem to generate all such cases and it has very limited value to list them. Is that a better way for me to think about this? Repository: rC Clang https://reviews.llvm.org/D54404 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54404: Exclude matchers which can have multiple results
steveire added inline comments. Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:624 + "hasAnyDeclaration", + "hasAnyName", + "hasAnyParameter", sbenza wrote: > I'm not sure what goes in this list. > `hasAnyName` is here but not `hasName`. > What is ambiguous about `hasAnyName`? I have a follow-up which adds output showing that `hasName` can be used. See http://ec2-52-14-16-249.us-east-2.compute.amazonaws.com:10240/z/_X9mnw If there was an entry there for `hasAnyName`, what would go in it? Repository: rC Clang https://reviews.llvm.org/D54404 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54404: Exclude matchers which can have multiple results
aaron.ballman added inline comments.
Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:606
+ // be used with particular arguments.
+ static std::vector ExcludedMatchers{
+ "allOf",
sbenza wrote:
> I don't think we are allowed to have non-trivial static storage duration
> objects like this.
> You have to use llvm::ManagedStatic. Or just make it a raw array.
IIRC, that's only needed for global statics, not local statics. Local statics
are less terrifying than their global brethren.
Repository:
rC Clang
https://reviews.llvm.org/D54404
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54404: Exclude matchers which can have multiple results
sbenza added a comment.
In https://reviews.llvm.org/D54404#1296224, @aaron.ballman wrote:
> In https://reviews.llvm.org/D54404#1295426, @steveire wrote:
>
> > I acknowledge and share the future-proofing concern.
> >
> > We could possibly use something trait-based instead and put the trait
> > beside the matcher definition in ASTMatchers.h, but that doesn't really
> > solve the problem. It only moves the problem.
>
>
> If we could somehow incorporate it into the matcher definition itself,
> though, it means we don't have two lists of things to keep updated. You're
> right that it doesn't eliminate the problem -- we still have to know to ask
> the question when reviewing new matchers (so perhaps something that requires
> you to explicitly opt-in/out would be better).
>
> Adding @sbenza in case he has ideas.
We could put something in `MatcherDescriptor` to indicate this. However, I am
still not sure what property are we looking for here.
Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:606
+ // be used with particular arguments.
+ static std::vector ExcludedMatchers{
+ "allOf",
I don't think we are allowed to have non-trivial static storage duration
objects like this.
You have to use llvm::ManagedStatic. Or just make it a raw array.
Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:624
+ "hasAnyDeclaration",
+ "hasAnyName",
+ "hasAnyParameter",
I'm not sure what goes in this list.
`hasAnyName` is here but not `hasName`.
What is ambiguous about `hasAnyName`?
Repository:
rC Clang
https://reviews.llvm.org/D54404
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54404: Exclude matchers which can have multiple results
steveire added a comment. Let's discuss it on IRC at some point and see if we can come up with wording. Repository: rC Clang https://reviews.llvm.org/D54404 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54404: Exclude matchers which can have multiple results
aaron.ballman added a reviewer: sbenza.
aaron.ballman added a subscriber: sbenza.
aaron.ballman added a comment.
In https://reviews.llvm.org/D54404#1295426, @steveire wrote:
> I acknowledge and share the future-proofing concern.
>
> We could possibly use something trait-based instead and put the trait beside
> the matcher definition in ASTMatchers.h, but that doesn't really solve the
> problem. It only moves the problem.
If we could somehow incorporate it into the matcher definition itself, though,
it means we don't have two lists of things to keep updated. You're right that
it doesn't eliminate the problem -- we still have to know to ask the question
when reviewing new matchers (so perhaps something that requires you to
explicitly opt-in/out would be better).
Adding @sbenza in case he has ideas.
Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:604
+ static std::vector excludedMatchers{
+ "allOf",
aaron.ballman wrote:
> aaron.ballman wrote:
> > `excludedMatchers` -> `ExcludedMatchers`
> Please add comments that explain why these are excluded and under what
> circumstances this list should be updated to add or remove items.
>
> Also: how do we ensure this list stays properly updated? We sometimes miss
> updating `RegistryMaps()`, so I'm a bit concerned that adding a second list
> in a different location will be inviting trouble.
The comment is a bit better but still not enough to tell me what's common to
all of these matchers. It sort of reads to me like "all the matchers with
overloads" since overloaded matchers can be ambiguous with particular
arguments, but that's not what I think you're going for here.
For instance, I don't see how the `anything()` matcher can be ambiguous when
used with particular arguments -- it doesn't accept arguments and it matches
literally everything.
Repository:
rC Clang
https://reviews.llvm.org/D54404
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54404: Exclude matchers which can have multiple results
steveire updated this revision to Diff 173766.
steveire added a comment.
Update
Repository:
rC Clang
https://reviews.llvm.org/D54404
Files:
lib/ASTMatchers/Dynamic/Registry.cpp
unittests/ASTMatchers/Dynamic/RegistryTest.cpp
Index: unittests/ASTMatchers/Dynamic/RegistryTest.cpp
===
--- unittests/ASTMatchers/Dynamic/RegistryTest.cpp
+++ unittests/ASTMatchers/Dynamic/RegistryTest.cpp
@@ -579,6 +579,8 @@
EXPECT_TRUE(!Contains(Matchers, "functionDecl()"));
EXPECT_TRUE(Contains(Matchers, "cxxMethodDecl()"));
+
+ EXPECT_TRUE(!Contains(Matchers, "has()"));
}
} // end anonymous namespace
Index: lib/ASTMatchers/Dynamic/Registry.cpp
===
--- lib/ASTMatchers/Dynamic/Registry.cpp
+++ lib/ASTMatchers/Dynamic/Registry.cpp
@@ -601,13 +601,55 @@
Registry::getMatchingMatchers(ast_type_traits::ASTNodeKind StaticType) {
std::vector Result;
+ // Exclude matchers which can't unambiguously
+ // be used with particular arguments.
+ static std::vector ExcludedMatchers{
+ "allOf",
+ "anyOf",
+ "anything",
+ "containsDeclaration",
+ "eachOf",
+ "equalsNode",
+ "findAll",
+ "forEach",
+ "forEachConstructorInitializer",
+ "forEachDescendant",
+ "forEachOverridden",
+ "forEachSwitchCase",
+ "has",
+ "hasAncestor",
+ "hasAnyArgument",
+ "hasAnyConstructorInitializer",
+ "hasAnyDeclaration",
+ "hasAnyName",
+ "hasAnyParameter",
+ "hasAnySelector",
+ "hasAnySubstatement",
+ "hasAnyTemplateArgument",
+ "hasAnyUsingShadowDecl",
+ "hasArgumentOfType",
+ "hasDescendant",
+ "hasEitherOperand",
+ "hasMethod",
+ "hasParent",
+ "isExpansionInFileMatching",
+ "isSameOrDerivedFrom",
+ "matchesName",
+ "matchesSelector",
+ "unless"};
+ assert(std::is_sorted(ExcludedMatchers.begin(), ExcludedMatchers.end()));
+
std::vector AcceptedTypes{StaticType};
processAcceptableMatchers(
- AcceptedTypes,
- [](StringRef Name, const MatcherDescriptor &,
-std::set &, std::vector>,
-unsigned) { Result.emplace_back((Name + "()").str()); });
+ AcceptedTypes, [](StringRef Name, const MatcherDescriptor &,
+ std::set &,
+ std::vector>, unsigned) {
+if (!std::binary_search(ExcludedMatchers.begin(),
+ExcludedMatchers.end(), Name)) {
+ Result.emplace_back((Name + "()").str());
+}
+ });
return Result;
}
Index: unittests/ASTMatchers/Dynamic/RegistryTest.cpp
===
--- unittests/ASTMatchers/Dynamic/RegistryTest.cpp
+++ unittests/ASTMatchers/Dynamic/RegistryTest.cpp
@@ -579,6 +579,8 @@
EXPECT_TRUE(!Contains(Matchers, "functionDecl()"));
EXPECT_TRUE(Contains(Matchers, "cxxMethodDecl()"));
+
+ EXPECT_TRUE(!Contains(Matchers, "has()"));
}
} // end anonymous namespace
Index: lib/ASTMatchers/Dynamic/Registry.cpp
===
--- lib/ASTMatchers/Dynamic/Registry.cpp
+++ lib/ASTMatchers/Dynamic/Registry.cpp
@@ -601,13 +601,55 @@
Registry::getMatchingMatchers(ast_type_traits::ASTNodeKind StaticType) {
std::vector Result;
+ // Exclude matchers which can't unambiguously
+ // be used with particular arguments.
+ static std::vector ExcludedMatchers{
+ "allOf",
+ "anyOf",
+ "anything",
+ "containsDeclaration",
+ "eachOf",
+ "equalsNode",
+ "findAll",
+ "forEach",
+ "forEachConstructorInitializer",
+ "forEachDescendant",
+ "forEachOverridden",
+ "forEachSwitchCase",
+ "has",
+ "hasAncestor",
+ "hasAnyArgument",
+ "hasAnyConstructorInitializer",
+ "hasAnyDeclaration",
+ "hasAnyName",
+ "hasAnyParameter",
+ "hasAnySelector",
+ "hasAnySubstatement",
+ "hasAnyTemplateArgument",
+ "hasAnyUsingShadowDecl",
+ "hasArgumentOfType",
+ "hasDescendant",
+ "hasEitherOperand",
+ "hasMethod",
+ "hasParent",
+ "isExpansionInFileMatching",
+ "isSameOrDerivedFrom",
+ "matchesName",
+ "matchesSelector",
+ "unless"};
+ assert(std::is_sorted(ExcludedMatchers.begin(), ExcludedMatchers.end()));
+
std::vector AcceptedTypes{StaticType};
processAcceptableMatchers(
- AcceptedTypes,
- [](StringRef Name, const MatcherDescriptor &,
-std::set &, std::vector>,
-unsigned) { Result.emplace_back((Name + "()").str()); });
+ AcceptedTypes, [](StringRef Name, const MatcherDescriptor &,
+ std::set &,
+ std::vector>, unsigned) {
+if (!std::binary_search(ExcludedMatchers.begin(),
+
[PATCH] D54404: Exclude matchers which can have multiple results
steveire added a comment. I acknowledge and share the future-proofing concern. We could possibly use something trait-based instead and put the trait beside the matcher definition in ASTMatchers.h, but that doesn't really solve the problem. It only moves the problem. Repository: rC Clang https://reviews.llvm.org/D54404 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54404: Exclude matchers which can have multiple results
aaron.ballman added inline comments.
Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:604
+ static std::vector excludedMatchers{
+ "allOf",
`excludedMatchers` -> `ExcludedMatchers`
Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:604
+ static std::vector excludedMatchers{
+ "allOf",
aaron.ballman wrote:
> `excludedMatchers` -> `ExcludedMatchers`
Please add comments that explain why these are excluded and under what
circumstances this list should be updated to add or remove items.
Also: how do we ensure this list stays properly updated? We sometimes miss
updating `RegistryMaps()`, so I'm a bit concerned that adding a second list in
a different location will be inviting trouble.
Repository:
rC Clang
https://reviews.llvm.org/D54404
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54404: Exclude matchers which can have multiple results
steveire created this revision.
steveire added a reviewer: aaron.ballman.
Herald added a subscriber: cfe-commits.
Repository:
rC Clang
https://reviews.llvm.org/D54404
Files:
lib/ASTMatchers/Dynamic/Registry.cpp
unittests/ASTMatchers/Dynamic/RegistryTest.cpp
Index: unittests/ASTMatchers/Dynamic/RegistryTest.cpp
===
--- unittests/ASTMatchers/Dynamic/RegistryTest.cpp
+++ unittests/ASTMatchers/Dynamic/RegistryTest.cpp
@@ -579,6 +579,8 @@
EXPECT_TRUE(!Contains(Matchers, "functionDecl()"));
EXPECT_TRUE(Contains(Matchers, "cxxMethodDecl()"));
+
+ EXPECT_TRUE(!Contains(Matchers, "has()"));
}
} // end anonymous namespace
Index: lib/ASTMatchers/Dynamic/Registry.cpp
===
--- lib/ASTMatchers/Dynamic/Registry.cpp
+++ lib/ASTMatchers/Dynamic/Registry.cpp
@@ -601,15 +601,54 @@
Registry::getMatchingMatchers(ast_type_traits::ASTNodeKind StaticType) {
std::vector Result;
+ static std::vector excludedMatchers{
+ "allOf",
+ "anyOf",
+ "anything",
+ "containsDeclaration",
+ "eachOf",
+ "equalsNode",
+ "findAll",
+ "forEach",
+ "forEachConstructorInitializer",
+ "forEachDescendant",
+ "forEachOverridden",
+ "forEachSwitchCase",
+ "has",
+ "hasAncestor",
+ "hasAnyArgument",
+ "hasAnyConstructorInitializer",
+ "hasAnyDeclaration",
+ "hasAnyName",
+ "hasAnyParameter",
+ "hasAnySelector",
+ "hasAnySubstatement",
+ "hasAnyTemplateArgument",
+ "hasAnyUsingShadowDecl",
+ "hasArgumentOfType",
+ "hasDescendant",
+ "hasEitherOperand",
+ "hasMethod",
+ "hasParent",
+ "isExpansionInFileMatching",
+ "isSameOrDerivedFrom",
+ "matchesName",
+ "matchesSelector",
+ "unless"};
+ assert(std::is_sorted(excludedMatchers.begin(), excludedMatchers.end()));
+
std::vector AcceptedTypes;
AcceptedTypes.push_back(StaticType);
processAcceptableMatchers(
AcceptedTypes, [](StringRef Name, const MatcherDescriptor
,
std::set ,
std::vector> ArgsKinds,
unsigned MaxSpecificity) {
-Result.emplace_back((Name + "()").str());
+if (!std::binary_search(excludedMatchers.begin(),
+excludedMatchers.end(), Name)) {
+ Result.emplace_back((Name + "()").str());
+}
});
return Result;
Index: unittests/ASTMatchers/Dynamic/RegistryTest.cpp
===
--- unittests/ASTMatchers/Dynamic/RegistryTest.cpp
+++ unittests/ASTMatchers/Dynamic/RegistryTest.cpp
@@ -579,6 +579,8 @@
EXPECT_TRUE(!Contains(Matchers, "functionDecl()"));
EXPECT_TRUE(Contains(Matchers, "cxxMethodDecl()"));
+
+ EXPECT_TRUE(!Contains(Matchers, "has()"));
}
} // end anonymous namespace
Index: lib/ASTMatchers/Dynamic/Registry.cpp
===
--- lib/ASTMatchers/Dynamic/Registry.cpp
+++ lib/ASTMatchers/Dynamic/Registry.cpp
@@ -601,15 +601,54 @@
Registry::getMatchingMatchers(ast_type_traits::ASTNodeKind StaticType) {
std::vector Result;
+ static std::vector excludedMatchers{
+ "allOf",
+ "anyOf",
+ "anything",
+ "containsDeclaration",
+ "eachOf",
+ "equalsNode",
+ "findAll",
+ "forEach",
+ "forEachConstructorInitializer",
+ "forEachDescendant",
+ "forEachOverridden",
+ "forEachSwitchCase",
+ "has",
+ "hasAncestor",
+ "hasAnyArgument",
+ "hasAnyConstructorInitializer",
+ "hasAnyDeclaration",
+ "hasAnyName",
+ "hasAnyParameter",
+ "hasAnySelector",
+ "hasAnySubstatement",
+ "hasAnyTemplateArgument",
+ "hasAnyUsingShadowDecl",
+ "hasArgumentOfType",
+ "hasDescendant",
+ "hasEitherOperand",
+ "hasMethod",
+ "hasParent",
+ "isExpansionInFileMatching",
+ "isSameOrDerivedFrom",
+ "matchesName",
+ "matchesSelector",
+ "unless"};
+ assert(std::is_sorted(excludedMatchers.begin(), excludedMatchers.end()));
+
std::vector AcceptedTypes;
AcceptedTypes.push_back(StaticType);
processAcceptableMatchers(
AcceptedTypes, [](StringRef Name, const MatcherDescriptor ,
std::set ,
std::vector> ArgsKinds,
unsigned MaxSpecificity) {
-Result.emplace_back((Name + "()").str());
+if (!std::binary_search(excludedMatchers.begin(),
+excludedMatchers.end(), Name)) {
+ Result.emplace_back((Name + "()").str());
+}
});
return Result;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
