Fix #200 Add support for copying Uri structures #230
Conversation
There was a problem hiding this comment.
@kocsismate I'm only starting to have a closer look. From what I see so far:
- the pull request does multiple things in a single commit: e.g. move code around and add new code — that's too much for a single commit and the move itself does not seem strictly necessary to me as exposing needed functions through their related
.hfiles would also make them available for use and then the move is fully optional (and something I would want to do as a potential refactoring myself later to keep this pull request to the point). I would ask to revert/drop all moves and expose through existing headers instead, and to add the new copy code to new filesUriCopy.{c,h}, and to then get back to detail level review. If some move is more necessary than I understood, it should be necessary moves only for now and in a dedicated commit please. Thank you! - The way that function
PreventLeakageis used looks mistaken: therevertMaskinput would need to grow with each allocation a ladoneMask |= URI_NORMALIZE_SCHEME;—git grep -F '|=' -- src/UriNormalize.cyields examples of that.
I'll pause review here for now, waiting for feedback.
Thanks for the input! I'll try to address your suggestion ASAP, possibly around Wednesday! |
@kocsismate cool! 👍 |
976a2b0 to
e9264b2
Compare
There was a problem hiding this comment.
@kocsismate flushing some comments from review and taking a break from review for new, more to come…
src/UriNormalize.c
Outdated
| URI_CHAR * dup = memory->malloc(memory, lenInBytes); | ||
| if (dup == NULL) { | ||
| return URI_FALSE; /* Raises malloc error */ | ||
| if (URI_FUNC(CopyRangeEngine)(range, range, memory) == URI_FALSE) { |
There was a problem hiding this comment.
Please make this refactoring "extract function" a dedicated first separate commit. Files UriCommon{c,h} seem like a good target for this. I'll comment on the function name further up.
There was a problem hiding this comment.
I'll reorder my commits a little bit later, when the code is about to be finalized. :)
Yes, I had a similar plan in mind too! :) |
There was a problem hiding this comment.
@kocsismate I have yet to review the big loop. Flushing some comments…
There was a problem hiding this comment.
@kocsismate the loop needs more work. Please see details including a draft for an alternative approach with ideas to re-use or pick from below…
|
@kocsismate PS: I have gone forward merging #231 just now because…
I hope that's good with you? |
Yes, thank you! I'm happy with the merge. I don't think I'm able to review it meaningfully, but I'll have a bit deeper look at it soon. |
I agree, I needed quite some time to come up with it and understand it. So I'm happy with your suggestion. I'll try to implement it asap, but I'm on vacation, so my response time may be even more hectic than usuallly. |
|
@kocsismate I'm looking forward to the next iteration. No need to hectify your vacation for me. I myself will also have some (on-)off-screen time in the coming 10 days. My worst expected response time will probably be 48 hours. |
|
As far as I see, all the existing review comments are now addressed. I'm now working on the tests. |
There was a problem hiding this comment.
@kocsismate I'm on the road with small screen, just two quick findings for now, more later:
There was a problem hiding this comment.
@kocsismate made time for more review now, here's what I found:
d4196ea to
eb17493
Compare
|
@hartwork I should have fixed all your comments by now. I'll wait until you have the chance to review my latest commit, and then I'll reorder my commits by creating a separate commit for the |
There was a problem hiding this comment.
@kocsismate thanks for the adjustments and extensions, here is what I found more:
There was a problem hiding this comment.
@kocsismate thanks for the adjustments! I'd like to slightly change the memcmp lines as following. Part of why it feels odd as-is is because sizeof(char) is defined to be 1 by C89. That made me consider it more:
…-copying copying: Add missing initial reset to destUri (follow-up to #230)
Do not copy `.hostData.ipFuture` unnecessarily (follow-up to #230)
I also need this functionality for https://wiki.php.net/rfc/url_parsing_api because of multiple reasons:
Uriclass exposes both the normalized and non-normalized URI components (therefore it internally stores two Uri structs). When a normalized component is requested, normalization is done on demand, by copying the original struct firstThe patch needs a last few touches, including tests, but I wanted to propose it nevertheless to get early feedback.