Property get/set implementation #32
Conversation
| /* Returns a new string of combined strings */ | ||
| char *strcatalloc(const char *a, const char *b) /* {{{ */ | ||
| { | ||
| char *out = estrndup(a, strlen(a) + strlen(b)+1); |
There was a problem hiding this comment.
This is wrong. This will try to read more data from a than allocated. Use emalloc+ 2xmemcpyorsuch ...
|
I've added a few quick notes on some small code issues, without doing a proper review. Please mind: Discussion about such a big feature should happen on internals, not here, so everybody is involved. |
|
It was recommended that I do a pull request by Alexey Shein... getting mixed signals on appropriate procedure... |
|
A pull request is nice for people to test it out and lateron (maybe) merge it. It's ok to discuss the actual implementation. For discussions about the concept/design it is bad, as not everybody is watching these and it's hard to follow (missing threading etc.) |
|
Alright... so not sure which way to go here. I've attempted to start discussion on the implementation several times over the last few months with little response. The original RFC was written two years ago and I was told by that author that it was vetted pretty heavily. With rare exception I've followed that RFC closely. So after I address these issues, should I cancel this pull request and re-submit or do something else? |
|
I guess I see that the new commits automagically end up here. Let me know how you want me to proceed. AFAIK everything you mentioned here has been addressed. |
|
There are a tonne of whitespace changes in this PR, should they be there? |
|
Unless something converted them incorrectly, yeah they should be there. Do you have a specific section you're concerned about?
|
| { \ | ||
| zn.op_type = IS_CONST; \ | ||
| INIT_PZVAL(&zn.u.constant); \ | ||
| ZVAL_STRINGL(&zn.u.constant, str, l;, 1); \ |
There was a problem hiding this comment.
that ';' on line 50 is obviously a typo
There was a problem hiding this comment.
Good catch, I wonder how it even compiles
-----Original Message-----
From: Anatoliy Belsky [mailto:[email protected]]
Sent: Wednesday, July 04, 2012 9:43 AM
To: Clint Priest
Subject: Re: [php-src] Property get/set implementation (#32)@@ -34,6 +34,23 @@
#define FREE_PNODE(znode) zval_dtor(&znode->u.constant);
+#define IS_ACCESSOR(purpose) ( purpose >= ZEND_FNP_PROP_GETTER && purpose <= ZEND_FNP_PROP_UNSETTER )
+
+#define MAKE_ZNODE(zn, str) \
- { \
zn.op_type = IS_CONST; \INIT_PZVAL(&zn.u.constant); \ZVAL_STRINGL(&zn.u.constant, str, strlen(str), 1); \zn.EA = 0; \- }
+#define MAKE_ZNODEL(zn, str, l) \- { \
zn.op_type = IS_CONST; \INIT_PZVAL(&zn.u.constant); \ZVAL_STRINGL(&zn.u.constant, str, l;, 1); \that ';' on line 50 is obviously a typo
Reply to this email directly or view it on GitHub:
https://github.com/php/php-src/pull/32/files#r1100500
There was a problem hiding this comment.
That's simple ... macro expansion. This becomes
/* ... _/
const char *__s=(str); int __l=l;;
zval *_z = (&zn.u.constant);
/ ... */
before the compiler even sees it ... and the extra ; is valid.
Macros are evil (but we'd need C++ templates as feasible alternative, but we're doing C ...)
|
There is another place where it'll be unhappy in that macros I think, Z_STRVAL_P(z) = (duplicate?estrndup(__s, ____l;):(char*)__s);\ |
| /* Add $value parameter to __setHours() */ | ||
| znode unused_node, unused_node2, value_node; | ||
| unused_node.op_type = unused_node2.op_type = IS_UNUSED; | ||
| unused_node.u.op.num = unused_node2.u.op.num = 1; |
There was a problem hiding this comment.
One more place where declaration should go upper )
There was a problem hiding this comment.
What do you mean by go upper? What line are you referring to?
There was a problem hiding this comment.
Line 1705 ==> znode unused_node, unused_node2, value_node;
This should go up to the line 1693 at least. When it's synced with master, I'll test it today )
There was a problem hiding this comment.
C89 says in §3.6.2
compound-statement:
{ declaration-list<opt> statement-list<opt> }
Or in other words: In a code block enclosed by { } variable declarations go first, then statments. So this is valid:
{ int a; foo(); }
whereas
{ foo(); int a; }
is invalid. This is different in C++ and C99.
There was a problem hiding this comment.
Ah right, you mentioned this before and I fixed a bunch of these. Not sure if I missed this or forgot again, my apologies. My fork is synced with master as of this morning, but isset/unset accessors are not completely working right. What is checked in is a partial implementation of the isset/unset accessors but there are some contexts it won't work in, everything else should be 100% unless something merged in from master broke something I've done.
I'm hoping to be able to spend an hour or so a day again on this project to get it completed, been working on it almost a year now. :)
There was a problem hiding this comment.
Hi,
did you push that sync because I can't see any commits about that ) .. or do I miss something?
There was a problem hiding this comment.
Hmm, well I thought it had. I had checked against a recent commit from the main repo against what the fork on github showed and I saw the same changes but I just checked another change and I'm not seeing them now. I'll poke around some more and see what's up. Not super familiar with git just yet.
There was a problem hiding this comment.
Alright, showing merge/commit now on my github fork, also took care of the variable declaration moving to beginning of block (unused node).
|
May be you could merge the current master into your branch? As there are a couple of rejected files because master seems to have some stuff from this patch already. I could test it then. |
|
Alright, it's caught back up with master. I'll work on these comments this morning. The isset/unset accessors functionality is mostly completed but I need to see where I left off and finish that up. |
|
I've run the compilation on windows, it's broken, here are the compiler messages Zend/zend_language_scanner.c(5741) : error C2045: 'yy571' : label redefined and so on about 20 lines. Not sure if only the first line counts. Also, I din't try on linux, but it's probably the same there. Ah, this happens either when I pull from your repo into my working tree or when I clone your repo, so the same on both. Another thing is I've discovered - as github says in it's docs, one get get just a patch using an URL like https://github.com/php/php-src/pull/32.patch ... but when I look at it, it's too old. github has bugs too :) EDIT: indeed, when i look into Zend/zend_language_scanner.c , the warned labels are defined twice. |
|
Do you have the re2c available? Sounds like the zend_language_scanner.c file needs to be recompiled... I ran into this when I first started on this project...
|
|
I've just cloned your branch on linux and did a fresh compile, aka "./buildconf && ./configure && make" ... and I see the same Zend/zend_language_scanner.c: In function ‘lex_scan’: As your branch is synced with master now, the same were after the pull request were merged. On win re2c is of course available with the php-sdk. Could you give me an advise how to regenerate the scanner code? I'll try then to compile on linux and windows again. |
|
Got it, but that's strange. Just took the corresponding line from the Makefile and ran it manually re2c --case-inverted -cbdFt Zend/zend_language_scanner_defs.h -oZend/zend_language_scanner.c Zend/zend_language_scanner.l so will test further ) |
|
Glad it was resolved. At the time I ran into a similar problem I was told to just delete the zend_language_scanner.c file and it would be regenerated... LMK how the testing goes and thank you! I have written numerous test cases: tests/classes/accessor_final_inner_error.phpt There are also some test cases for the php/reflection extension.
|
|
The first run is actually very good - no test fails on linux and windows )) Despite that, when run with valgrind on linux, all tests are shown as leaked. But it's almost the same line. I've uploaded some valgrind traces, you'll find that under http://belsky.info/phpz/cpriest/ |
|
Thanks for the traces. Can you tell me the command line you used to produce these so I can test to be sure they are resolved on my end? This could be due to the currently "mixed state," Some of the committed code includes changes to allow isset/unset accessor and not all use cases are covered yet. I am in the middle of a slight re-write of the auto-implementation code which could be where this has come from.
|
|
Actually the param -m is responsible for the memory checks To run a particular test do To debug it, you could do first Let me know when I can test it again :) |
|
I've been working pretty heavily on this again, nearly complete. There have been some major changes. I've also been reading the pro git book. It's been suggested that I rebase the code and I was thinking of "starting over" with a fresh fork, creating a branch and applying my commits as a rebase on that branch, push back up here and go from there. What would you suggest? Would that mean killing this pull request and starting a new one? Would these comments be gone? |
|
Alright, was getting back to the -m tests. If I run all of my tests through run-tests.php using the -m option, they all "PASS". I've seen valgrind output before and I don't speak it, I haven't seen any output from the latest -m run-through, does this mean all of my tests pass "valgrind" as well or is there some other output I should be looking for to see that valgrind is happy? Only things remaining on this are to update reflection and add a couple of requested functions. Then I believe this very large patch is feature-complete, unless the latest round of RFC comes up with yet more requests. |
|
If -m shows no leaked tests it should be fine. To be sure you cover everything in your code your could once produce coverage if you have time. I'm going to test your branch again than ) |
|
Okay, all of your comments have been addressed. I also went over the code looking for other ANSI compliance issues, there were a number of them. Just not used to single line { } coding styles. With your PHP coding standard, are you referring to the test cases or the ANSI stuff? Lastly, can you point me in some direction on how to get code coverage analysis? no idea how to do that with linux/gcc/php-core |
|
For reasons I can't explain right now, doing a run-tests with valgrind is showing a LEAK on all standard tests, none of my tests though... I was pretty sure I did a full valgrind test before all of these commits... ==32198== Conditional jump or move depends on uninitialised value(s) Any idea what's going on here? These line numbers don't really correspond to anything special... |
|
A one thing seems to be still overseen cpriest@98f51c8#zend-zend_compile-c-P489 , it's zend_compile.c:7853 now. After correcting this it compiles fine on windows. All the tests pass :) With the coverage - that's just a recomendaditon. That might require some effort, but if you've got time, there is a wiki page https://wiki.php.net/qa/testcoverage . You can see how it looks like for instance here http://gcov.php.net/PHP_5_4/lcov_html/ . Once it's set up you could see which code parts could/need to be tested more. |
|
ah, and the PHP coding standard is here https://git.php.net/?p=php-src.git;a=blob_plain;f=CODING_STANDARDS;hb=HEAD |
|
Hm, I don't see how that leaks could be related to the latest two commits. But it's easy to revert them and see if there was no leaks before. Heh ... |
|
Yeah, I don’t get it, I was reverting, back several versions and they were all causing the leak. I’ll have to track it down, dunno what the output of valgrind is really saying as the lines don’t line up to anything in particular and it shows its happening in zend_api.h From: Anatoliy Belsky [mailto:[email protected]] Hm, I don't see how that leaks could be related to the latest two commits. But it's easy to revert them and see if there was no leaks before. Heh ... — |
|
I’ll read this over again, I thought I’d followed most of it but have been working on this piece meal for over a year (and a house move and a new baby. ☺) With that other ANSI mention, I had already fixed that with the two last commits. I’ll see about the code coverage as I’d like it to be well tested. Unfortunately it looks like the latest RFC discussions has really churned up a hornets nest and may need to have a lot more additional work/re-work done. Not even sure I’m up for it at this point. A one thing seems to be still overseen cpriest/php-src@98f51c8#zend-zend_compile-c-P489cpriest@98f51c8#zend-zend_compile-c-P489 , it's zend_compile.c:7853 now. After correcting this it compiles fine on windows. All the tests pass :) With the coverage - that's just a recomendaditon. That might require some effort, but if you've got time, there is a wiki page https://wiki.php.net/qa/testcoverage . You can see how it looks like for instance here http://gcov.php.net/PHP_5_4/lcov_html/ . Once it's set up you could see which code parts could/need to be tested more. From: Anatoliy Belsky [mailto:[email protected]] ah, and the PHP coding standard is here https://git.php.net/?p=php-src.git;a=blob_plain;f=CODING_STANDARDS;hb=HEAD — |
|
How irritating. This was because I had (back in September) added –disable-cgi and –disable-phar to the ./configure options in order to speed up compliation. So… the run-tests.php was using the php-cgi on some tests which was compiled back in september… That was hard to track down because all of the valgrind line numbers made no sense. Ah well, all tests passing valgrind now, like on your side. There has been major discussion over the RFC, yet again, so there is likely to be a slew of new changes to this pull request. Do you mind keeping the pull request here or do you want to start over when the next round of deliberations has concluded and the code re-written? From: Anatoliy Belsky [mailto:[email protected]] Hm, I don't see how that leaks could be related to the latest two commits. But it's easy to revert them and see if there was no leaks before. Heh ... — |
|
Yep, the new round is looking to be hot right now :) ... As for me, keeping this pull request is ok, just ping here when you need the code to be tested again. |
|
Sounds good. Honestly these things being discussed are quite significant and I may just scratch the whole fork and start over. I’ve learned a lot in the last year. ☺ That may be extreme, I’m not certain just yet but I want to let the deliberations come to a consensus before I work on the code any more. From: Anatoliy Belsky [mailto:[email protected]] Yep, the new round is looking to be hot right now :) ... As for me, keeping this pull request is ok, just ping here when you need the code to be tested again. — |
|
I am impressed by your pull. Really good work. I do have one concern about "read-only" and "write-only", wouldn't "readonly" and "writeonly" or "read_only" and "write_only" be more friendly for syntax-highlighting in editors and for any 3rd party syntax parser? |
|
Thanks, the read-only/write-only seems to have no fans and is probably going to be removed anyways. I chose the dash because I_hate_underscore_programming and I also really don't like wordsruntogether. :) But it probably won't matter, the group prefers to not add a new keyword and to instead have people put in a private final set(); if the functionality is needed, even though it will not be the same result, I could not convince them otherwise. |
32 tests to test the new functionality and edge cases
Updates to the Reflection extension to expose access to the accessors
Tests for the changes to the Reflection extension
Implementation notes tracked here:
https://wiki.php.net/rfc/propertygetsetsyntax-as-implemented