ext/tidy: minor refactorings #19167
ext/tidy: minor refactorings #19167
Conversation
Bring closer to unique call site and return value of it
Pass zend_string* along
| } else { | ||
| return (void *) ZSTR_EMPTY_ALLOC(); | ||
| } | ||
| if (*type == TidyString) { |
There was a problem hiding this comment.
what s wrong with switch/case ? for only 3 possibilities (and it is not going to change anytime soon), I m not against just to see if this is just taste or anything.
There was a problem hiding this comment.
The compiler complains if you don't add a return at the end of the function that is unreachable... so I thought forcing it to be 3 branches would be better. But can change back to a switch statement.
There was a problem hiding this comment.
the only thing weird I found with the existing code is the break statement after return but usually for enums comparisons especially when more than 2/3 values I prefer switch/case but if the situation forces if/else instead then I m not stubborn about it.
| TidyBuffer buf; | ||
|
|
||
| if(enc) { | ||
| ZEND_ASSERT(!ZEND_SIZE_T_UINT_OVFL(ZSTR_LEN(string))); |
There was a problem hiding this comment.
do not know if an exception should be thrown instead or this. Early morning review so forgive me if I miss something obvious :)
There was a problem hiding this comment.
This was always checked before calling this function, just making sure it stays that way :)
|
except comments, changes make sense overall. |
2 participants
No description provided.