-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add option to include method modifiers #5
Add option to include method modifiers #5
Conversation
@Jongy Please review |
Can you create meanwhile the gProfiler PR that uses it, so I can view how it all connects? |
Yes: intel/gprofiler#712 |
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 few comments
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.
2 comments
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.
One more small comment :)
I tested it locally but maybe better to wait for: https://github.com/Granulate/gprofiler/pull/712/checks |
src/frameName.h
Outdated
bool _includemm; | ||
// Based on: https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#:~:text=Table%C2%A04.5.%C2%A0Method%20access%20and%20property%20flags | ||
// Good practice order from: https://checkstyle.sourceforge.io/config_modifier.html#ModifierOrder | ||
const std::pair<int, std::string> access_flags [12] = { |
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.
Compiler complains about it:
src/frameName.h:94:5: warning: non-static data member initializers only available with -std=c++11 or -std=gnu++11 [enabled by default]
I suppose that's why you added -std=c++11
.
Can we make this static or move out of the class? It can also be defined in the .cpp
file.
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.
Making it static makes sure - we don't want to initialize this field for every instance of FrameName
.
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 added -std=c++11 because initializing vector of pairs resulted in compilation error. I'll check if it will work for static
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.
We can keep it array of pairs, but static :)
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.
Or even array of structs is fine too.
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.
Changed it to static and moved from header to source file. Also it seems that if it is static it can be initialized without [12]
without any errors
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.
.
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.
Merging
async-profiler side of intel/gprofiler#570.
async-profiler side of intel/gprofiler#570.
async-profiler side of intel/gprofiler#570.
async-profiler side of intel/gprofiler#570.
Connected with: intel/gprofiler#570