Closed nitsanw closed 8 years ago
Thanks, @nitsanw. I'll have a look in the next few days.
Hi @jrudolph, I understand you are probably busy and I realize this is not the cleanest PR. Is there anything I can do to help the review process along? I would like to invest more time in this project and build further on the above changes to make PMA more robust. Thanks, Nitsan
ping?
@nitsanw, thanks again. I like many of your suggestions but I think we need to break them down a bit more to see better what's going on. Unfortunately, there was another PR open which contained similar cleanups as yours to sig_string
, so that your changes won't apply any more.
Let's have those changes:
Could you open separate issues / PRs for those items, especially for the first item?
To give a few general guidelines about this project:
sig_string
improved there's not much left. In theory, (*vm)->GetEnv
could fail which would probably lead to segfaults but this is very unlikely to happen.Anyway, thanks for sharing the wish to make perf-map-agent better!
I'll see if I can find the time to redo this PR as a series of smaller PRs. This is hard because all the changes sort of hit on the same file. Given the code is so small I hoped you'd go through the merge process. I realize you have little time for reviews/maintenance, have you considered introducing more contributors to the project?
@nitsanw no worries, if you don't get to submitting the PR(s). If you don't I will at least fix the bug myself and see what to do about the other issues.
@nitsanw I added a few more changes similar to the ones you proposed. I didn't yet introduce compact_inline_format so if you'd like that feature please open a ticket to discuss it.
Thanks again. There was lots of good stuff in here.
Closing...
Glad you found it helpful. Will push further tweaks in near future.
Hi, This is a bit of a mixed bag, so feel free to cherry pick the bits you find most relevant. The most important aspects are:
In addition I added some comments, renamed variables for clarity (I think), and added a more compact format for inlined methods printing. Happy to have a discussion to make the PR more palatable. I would like to improve formatting further and settle on good way to configure it, but not sure how important backwards compatibility is. Thanks, Nitsan