-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Added check for visibility attribute specifier #51
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple small changes requested here, and one point of clarification.
matchers/utilities.cpp
Outdated
case Visibility::ProtectedVisibility: | ||
info["visibility"] = "protected"; | ||
break; | ||
default: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather there not be a default
option. That way in case these enumerations change we get a compiler warning about them, instead of values going blank unexpectedly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
@@ -38,7 +38,9 @@ void FunctionInfo::run(const MatchFinder::MatchResult& Result) { | |||
auto function = Result.Nodes.getNodeAs<FunctionDecl>("func"); | |||
|
|||
// Do not process class methods here. | |||
if (llvm::dyn_cast_or_null<CXXMethodDecl>(function)) return; | |||
if (!_options._process_class_methods) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain a bit why this is necessary?
LGTM! |
This change improves Hyde with following capabilities -
visibility
andvisibility_explicit
attributes to the JSON method info