@Girgias@derickr so ValueError for alphabet and padding and false for everything else concerning decoding then .. noted I will change the wording then. And it makes encoding always returning a string which is better.
@derickr@Girgias what's easier to start with the code or the RFC process ? I presume both can be made in parallel. That's brand new territories from me so any advice is welcomed. Also since I do not know C I can create a branch and already add tests if that can help
@nyamsprod@Girgias An RFC is probably easier to write... but I have always found it easier for changes like this to also do the implementation, as that sometimes informs me what the RFC needs to contain.
@derickr@Girgias I will create a branch and add the tests files for the implementation I think that all I can do since I do not know C. On that note any reason base64_encode/decode tests are under the url folder and not the strings folder in the php-src codebase 🤔. I wanted to put the tests for base32 beside those of base64 so it seems strange to me
@derickr@Girgias I see base64_encode is getting an option argument to remove the padding ... should I already take this into consideration for base32_encode also or not ?
@derickr@Girgias I updated my php codebase and remove all non string function except for array_key_exists and unpack I hope to create a PR to the php-src master with the tests ported hopefully first week of april. then I will ask for karma to create/ copy/paste my RFC for base32_(encode|decode) I might add the base32.h file but still checking how to write that one
@derickr@Girgias nice ... should then ease my conversion. will try to finish that one so you can see/review my horrible code. Once stable I will tackle the base32_decode which is more complex IMHO
@nyamsprod@Girgias I had a very quick look, and it looks like a good start. I'll have a better look later, but two suggestions right away:
When you create any zend_string and don't use it beyond the function, you need to free up the allocated memory, (zend_string_release), otherwise you're leaking it.
If you run the tests with: php run-tests.php -m ext/standard it will run with valgrind enabled to check for this (you might have to install it).
@nyamsprod@Girgias
2. Functions like strcspn() that operate on C strings are not null safe. I am sure there will be a null safe implementation somewhere in PHP's API, but can't remember the name on the top of my head.
You use smart_strings for which are close to PHP strings, but it's likely going to be much faster to preallocate the result buffer once with the right size.
I'm traveling the next few days, so might take a while to properly review. But a great start.
Add comment