Skip to content
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 value field #29

Closed
wants to merge 1 commit into from
Closed

add value field #29

wants to merge 1 commit into from

Conversation

dhopkalo
Copy link
Contributor

No description provided.

@dhopkalo dhopkalo mentioned this pull request Sep 24, 2016
@BitOne
Copy link
Owner

BitOne commented Nov 7, 2016

Hey @drefixs , I like the idea!

Can you rebase your commit on master, so we won't see the diff from the memory leaks fix here ?

Thanks a lot !

@@ -448,12 +454,22 @@ void meminfo_zval_dump(php_stream * stream, char * frame_label, char * symbol_na
php_stream_printf(stream TSRMLS_CC, " \"type\" : \"%s\",\n", zend_get_type_by_const(Z_TYPE_P(zv)));
php_stream_printf(stream TSRMLS_CC, " \"size\" : \"%ld\",\n", meminfo_get_element_size(zv));

php_json_encode(&buf, zv, (int)0);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid this only works if the JSON extension is loaded, which is not guaranteed.

@BitOne
Copy link
Owner

BitOne commented Nov 7, 2016

@drefixs : Could you please provide an option or parameter so users that want to display value can provide this parameter. I'm afraid that displaying value by default will make the file far less easier to read.
But the feature is nice, and certainly useful.

@BitOne BitOne force-pushed the master branch 25 times, most recently from 5e00cf6 to 4d0010a Compare November 16, 2017 14:23
@BitOne BitOne force-pushed the master branch 7 times, most recently from 9a97514 to 0e4f884 Compare November 16, 2017 14:51
@BitOne BitOne closed this Nov 17, 2017
@BitOne
Copy link
Owner

BitOne commented Nov 17, 2017

Closed because the code changed too much since this PR was opened. But I keep the idea from #31 opened and will implemented latter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants