Page MenuHomePhabricator

Add a new AST matcher 'optionally'.
ClosedPublic

Authored by air20 on Jan 5 2020, 2:43 PM.

Details

Summary

This matcher matches any node and at the same time executes all its
inner matchers to produce any possbile result bindings.

This is useful when a user wants certain supplementary information
that's not always present along with the main match result.

Diff Detail

Event Timeline

air20 created this revision.Jan 5 2020, 2:43 PM
kristina added a subscriber: kristina.

Add some reviewers.

kristina edited subscribers, added: cfe-commits; removed: kristina.Jan 5 2020, 3:08 PM

Just to make sure I understand the purpose -- the goal here is to optionally match one or more inner matchers without failing even if none of the inner matchers match anything, and this is a different use case than anyOf() because that would fail when none of the inner matchers match?

clang/include/clang/ASTMatchers/ASTMatchers.h
2538

This needs to be registered in the dynamic matcher registry (Registry.cpp) as well. Also, you should regenerate the documentation by running clang\docs\tools\dump_ast_matchers.py.

I think this matcher could use some example uses in the documentation as well.

clang/lib/ASTMatchers/ASTMatchersInternal.cpp
362

You can elide the braces here.

air20 updated this revision to Diff 236510.Jan 6 2020, 9:18 PM

Fixed Registry.cpp and documentation as per comments.

air20 updated this revision to Diff 236513.Jan 6 2020, 9:35 PM

Included base commits that was missing in the previous diff.

air20 updated this revision to Diff 236514.Jan 6 2020, 9:40 PM
air20 marked an inline comment as done.
air20 marked an inline comment as done.Jan 6 2020, 9:42 PM
aaron.ballman accepted this revision.Jan 7 2020, 5:06 AM

LGTM aside from a minor nit. Do you need someone to commit on your behalf?

clang/lib/ASTMatchers/Dynamic/Registry.cpp
283

Why did this one move? Please keep the list sorted alphabetically.

This revision is now accepted and ready to land.Jan 7 2020, 5:06 AM
air20 marked an inline comment as done.Jan 7 2020, 9:01 AM

Just to make sure I understand the purpose -- the goal here is to optionally match one or more inner matchers without failing even if none of the inner matchers match anything, and this is a different use case than anyOf() because that would fail when none of the inner matchers match?

That’s exactly right.

LGTM aside from a minor nit. Do you need someone to commit on your behalf?

How do I submit a patch from here?

clang/lib/ASTMatchers/Dynamic/Registry.cpp
283

I think this wasn’t sorted correctly before.

LGTM aside from a minor nit. Do you need someone to commit on your behalf?

How do I submit a patch from here?

You would need git commit credentials for the project (https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access). If you don't have those credentials, I'm happy to commit on your behalf.

clang/lib/ASTMatchers/Dynamic/Registry.cpp
283

I think it was sorted properly before (i comes before S even if you ignore capitalization), but regardless, it's a change unrelated to the patch, which we usually ask to be a separate patch.

air20 marked an inline comment as done.Jan 7 2020, 7:58 PM

LGTM aside from a minor nit. Do you need someone to commit on your behalf?

How do I submit a patch from here?

You would need git commit credentials for the project (https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access). If you don't have those credentials, I'm happy to commit on your behalf.

I don't have commit access for now. It'd be great if you can submit it for me :D

clang/lib/ASTMatchers/Dynamic/Registry.cpp
283

I trust my Vim for this :) I think uppercase letters have a smaller ASCII value so they come first.

Line 354-355 are sorted correctly, for example.

REGISTER_MATCHER(isConstQualified);
REGISTER_MATCHER(isConstexpr);

I'd rather not upload another patch for this though.

aaron.ballman added inline comments.Jan 8 2020, 5:31 AM
clang/lib/ASTMatchers/Dynamic/Registry.cpp
283

I trust my Vim for this :) I think uppercase letters have a smaller ASCII value so they come first.

I think it depends on your sort criteria and whether you are sorting this list for humans or computers. I sort it for humans where i comes before s alphabetically because the goal of alphabetizing this list is for a human to quickly scan it visually. (Computer sort order is unimportant in this case -- these are all being stored into a hash map where the order does not matter.)

I'd rather not upload another patch for this though.

This is a change unrelated to the patch at hand, so it should be removed from this patch. If you feel strongly about changing this, please start a separate patch.

air20 updated this revision to Diff 236866.Jan 8 2020, 10:58 AM
air20 marked an inline comment as done.Jan 8 2020, 11:01 AM
air20 added inline comments.
clang/lib/ASTMatchers/Dynamic/Registry.cpp
283

Reverted. But I thought the purpose of sorting is to make it look better and maintain *one* canonical order. Not a big deal anyhow.

aaron.ballman closed this revision.Jan 8 2020, 11:10 AM

Thank you for the patch! I've commit on your behalf in 2823e91d55891e33a7a8b9a4016db4ec9e2765ae

LGTM too. I described a more ad-hoc implementation previously: https://steveire.wordpress.com/2018/11/20/composing-ast-matchers-in-clang-tidy/ but I prefer this implementation.