Function to encode strings - #3
Conversation
|
Thanks for your contribution @miguelleitao! I'm very interested in adding this functionality, since we do not have an encoding functionality yet. Please see my inline comments on your merge requests for details (will follow in a couple of minutes :-) |
ooxi
left a comment
ooxi
left a comment
There was a problem hiding this comment.
Needs some work, but looks promising! Please also add a test case for this functionality.
| return (size_t)(to - dest); | ||
| } | ||
|
|
||
| int encode_html_entities(char *dest, const char *src) { |
There was a problem hiding this comment.
Please return sitze_t instead of int
There was a problem hiding this comment.
Completely agree. I'll commit the correction.
| int encode_html_entities(char *dest, const char *src) { | ||
| char *to = dest; | ||
| for( const char *from = src ; *from ; from++ ) { | ||
| int i = 9999; |
There was a problem hiding this comment.
Please declare i as late as possible and do not assign a magic unused value to it
There was a problem hiding this comment.
Completely agree. I'll commit the correction.
| } | ||
| //if ( *from=='\r' || *from=='\n' ) continue; | ||
| for( i=0 ; i<sizeof NAMED_ENTITIES / sizeof *NAMED_ENTITIES ; i++ ) | ||
| if ( *from == NAMED_ENTITIES[i][1][0] ) break; |
There was a problem hiding this comment.
I don't really get this logic :-) are you comparing just the first character or am I missing something? Or is this sufficient because of the available entities (then please make sure this remains the case by adding an appropriate test case)
There was a problem hiding this comment.
Yes, I'm just comparing the first character. I found this sufficient for the final application I was developing. Maybe this encoder should be classified as an ASCII encoder instead of an UTF-8 encoder. I'll think about this during next week. Enlarging the length of the test can be done but this solution will still be inefficient since it uses a top-down search on an unsorted table.
If I won't improve the test, I'll try to keep the restriction clear to future users or developers.
Any way, I'll provide a test case.
| } | ||
| } | ||
| *to = 0; | ||
| return strlen(dest); |
There was a problem hiding this comment.
Since you know dest and to, no call to strlen is necessary
There was a problem hiding this comment.
Completely agree. I'll commit the correction.
|
Hi ooxi. Thank you for your reviews. I just pushed a new version of the encoding function.
|
|
Thanks for splitting your changes into topic commits, of which I already have merged several into master! The rest can be found in the feature/encode branch and will be merged as soon as I have had a chance to look at it :-) |
Was this ever merged? 😅 |
|
@gjtorikian unfortunately, no. But feel free to use it as is :-) |
3 participants
Added a function to encode strings.