Page MenuHomePhabricator

[ASTMatchers] [NFC] Fix code examples
ClosedPublic

Authored by MaskRay on Jan 17 2018, 4:27 PM.

Diff Detail

Repository
rC Clang

Event Timeline

MaskRay created this revision.Jan 17 2018, 4:27 PM

Don't forget that you also need to regenerate the HTML docs:

$ cd docs/tools # yes, cd
$ ./dump_ast_matchers.py

The documentation needs to be regenerated for this patch. One thing that seems to be inconsistent is with the "what gets matched" messages is that sometimes it includes extra adornments like curly braces and other times it does not. It might be good to pick a style and try to be more consistent with it.

include/clang/ASTMatchers/ASTMatchers.h
1792

I don't think this adds clarity.

1841

I don't think this adds clarity.

1851

I think this adds confusion.

MaskRay updated this revision to Diff 130644.Jan 19 2018, 10:03 AM

Revert some changes to address comments

The documentation needs to be regenerated for this patch. One thing that seems to be inconsistent is with the "what gets matched" messages is that sometimes it includes extra adornments like curly braces and other times it does not. It might be good to pick a style and try to be more consistent with it.

Do I need to do anything to re-generate the doc and check it into this revision? If so, can you show me the Doxygen generation instruction?

Reverted some changes as they may cause confusion which are pointed by you.

The documentation needs to be regenerated for this patch. One thing that seems to be inconsistent is with the "what gets matched" messages is that sometimes it includes extra adornments like curly braces and other times it does not. It might be good to pick a style and try to be more consistent with it.

Do I need to do anything to re-generate the doc and check it into this revision? If so, can you show me the Doxygen generation instruction?

Yes, the documentation for this is something that has to be generated manually. You should just have to execute clang/docs/tools/dump_ast_matchers.py to regenerate the documentation.

Reverted some changes as they may cause confusion which are pointed by you.

Thanks! I found one more minor nit with some example code, but this basically looks good aside from the documentation bit.

include/clang/ASTMatchers/ASTMatchers.h
803

Spurious semi-colon in the declaration.

MaskRay updated this revision to Diff 130672.Jan 19 2018, 11:54 AM

Address comment

MaskRay marked an inline comment as done.Jan 19 2018, 11:55 AM
MaskRay marked 3 inline comments as done.
MaskRay added a comment.EditedJan 19 2018, 11:59 AM

I am also not sure about this function: line 3548

/// \brief Matches \c FunctionDecls and \c FunctionProtoTypes that have a
/// specific parameter count.
///
/// Given
/// \code
///   void f(int i) {}
///   void g(int i, int j) {}
///   void h(int i, int j);
///   void j(int i);
///   void k(int x, int y, int z, ...);
/// \endcode
/// functionDecl(parameterCountIs(2))
///   matches void g(int i, int j) {}
/// functionProtoType(parameterCountIs(2))
///   matches void h(int i, int j)
/// functionProtoType(parameterCountIs(3))
///   matches void k(int x, int y, int z, ...);
AST_POLYMORPHIC_MATCHER_P(parameterCountIs,
                          AST_POLYMORPHIC_SUPPORTED_TYPES(FunctionDecl,
                                                          FunctionProtoType),
}

Both functionDecl and functionProtoType match these functions as long as the parameter count is matched.

% echo 'match functionDecl()'|clang-query =(printf 'void f(){}') -- -xc++
% echo 'match functionProtoType()'|clang-query =(printf 'void f(){}') -- -xc++

I am also not sure about this function: line 3548

/// \brief Matches \c FunctionDecls and \c FunctionProtoTypes that have a
/// specific parameter count.
///
/// Given
/// \code
///   void f(int i) {}
///   void g(int i, int j) {}
///   void h(int i, int j);
///   void j(int i);
///   void k(int x, int y, int z, ...);
/// \endcode
/// functionDecl(parameterCountIs(2))
///   matches void g(int i, int j) {}
/// functionProtoType(parameterCountIs(2))
///   matches void h(int i, int j)
/// functionProtoType(parameterCountIs(3))
///   matches void k(int x, int y, int z, ...);
AST_POLYMORPHIC_MATCHER_P(parameterCountIs,
                          AST_POLYMORPHIC_SUPPORTED_TYPES(FunctionDecl,
                                                          FunctionProtoType),
}

Both functionDecl and functionProtoType match these functions as long as the parameter count is matched.

% echo 'match functionDecl()'|clang-query =(printf 'void f(){}') -- -xc++
% echo 'match functionProtoType()'|clang-query =(printf 'void f(){}') -- -xc++

I'm not certain I'm following along. The quoted matcher is for parameterCountIs(), but the test code you've posted doesn't use that matcher. However, the quoted comment seems like it's wrong: functionDecl(parameterCountIs(2)) should match both g() and h(), not just g(), I believe.

I am also not sure about this function: line 3548

/// \brief Matches \c FunctionDecls and \c FunctionProtoTypes that have a
/// specific parameter count.
///
/// Given
/// \code
///   void f(int i) {}
///   void g(int i, int j) {}
///   void h(int i, int j);
///   void j(int i);
///   void k(int x, int y, int z, ...);
/// \endcode
/// functionDecl(parameterCountIs(2))
///   matches void g(int i, int j) {}
/// functionProtoType(parameterCountIs(2))
///   matches void h(int i, int j)
/// functionProtoType(parameterCountIs(3))
///   matches void k(int x, int y, int z, ...);
AST_POLYMORPHIC_MATCHER_P(parameterCountIs,
                          AST_POLYMORPHIC_SUPPORTED_TYPES(FunctionDecl,
                                                          FunctionProtoType),
}

Both functionDecl and functionProtoType match these functions as long as the parameter count is matched.

% echo 'match functionDecl()'|clang-query =(printf 'void f(){}') -- -xc++
% echo 'match functionProtoType()'|clang-query =(printf 'void f(){}') -- -xc++

I'm not certain I'm following along. The quoted matcher is for parameterCountIs(), but the test code you've posted doesn't use that matcher. However, the quoted comment seems like it's wrong: functionDecl(parameterCountIs(2)) should match both g() and h(), not just g(), I believe.

Yes, I was asking about this because the results seemed to be wrong. I should have used parameterCountIs() to reduce the confusion :) I'll leave them untouched.

Yes, I was asking about this because the results seemed to be wrong. I should have used parameterCountIs() to reduce the confusion :) I'll leave them untouched.

Alternatively, you can try out those declarations and the given matchers in clang-query to see if the results match what's listed, and update the comments based on what you find. Either way is fine.

MaskRay updated this revision to Diff 130711.Jan 19 2018, 3:43 PM

functionProtoType

Great, all that remains is for you to regenerate the documentation file and upload that with the next patch.

MaskRay updated this revision to Diff 130922.Jan 22 2018, 10:14 AM

Regenerate HTML docs

$ cd docs/tools # yes, cd
$ ./dump_ast_matchers.py

aaron.ballman accepted this revision.Jan 22 2018, 11:50 AM

LGTM, thank you! Do you need me to commit on your behalf?

This revision is now accepted and ready to land.Jan 22 2018, 11:50 AM
This revision was automatically updated to reflect the committed changes.