Less temporary memory allocations & share AF data when possible #632
Conversation
| if(tb->has_off && *tb->pos == 0) { | ||
| str_append_c(&txt, "OFF"); | ||
| snprintf(tb->txt + off, sizeof(tb->txt) - off, "%s", "OFF"); |
There was a problem hiding this comment.
I dislike this -- the idea with str type is that we can later convert it to utf-8. This is taking steps backwards. How about we cache the str to textslider instead ?
|
I think we can close this now ? |
|
no, I think there's still some valid changes here, and maybe a broader discussion about when we should be using |
Okay. I'll start then :) I'd rather use the str whenever it comes to text, since it abstracts away the buffer length management and makes the code look a bit easier to read. snprintf and friends especially make it really hard to read (or properly understand!) the code :/ |
There was a problem hiding this comment.
scene.c fighter de-duplication is a neat shortcut, seems worthwhile.
script.c changes reduce number of extremely small stack based strings we create from slices, but no longer effects allocator pressure. Unsure if text slider changes are still relevant, but maybe?
replacing str usage with snprintf is icky, especially after katajakasa added the small string optimization.. checks commit log a few hours after this PR was opened? x)
| str_from_slice(&test, src, *now, *now + m); | ||
| if(str_equal_c(&test, "usw") && find_numeric_span(src, *now + m) > *now + m) { | ||
| // str_from_slice(&test, src, *now, *now + m); | ||
| if(strncmp(src->data + *now, "usw", m) == 0 && find_numeric_span(src, *now + m) > *now + m) { |
There was a problem hiding this comment.
| if(strncmp(src->data + *now, "usw", m) == 0 && find_numeric_span(src, *now + m) > *now + m) { | |
| if(strncmp(str_c(src) + *now, "usw", 3) == 0 && find_numeric_span(src, *now + m) > *now + m) { |
The strncmp call should compare all 3 characters like the str_equal_c did before, not just the first m.
| @@ -215,7 +215,7 @@ bool str_to_long(const str *string, long *m); | |||
| * @return true If success | |||
| * @return false If failure (result value is invalid) | |||
| */ | |||
| bool str_to_int(const str *string, int *m); | |||
| bool str_to_int(const str *string, size_t offset, int *m); | |||
There was a problem hiding this comment.
Please document the new offset param.
| *result = clamp_long_to_int(value); | ||
| bool str_to_int(const str *string, size_t pos, int *result) { | ||
| char *end; | ||
| *result = strtol(string->data + pos, &end, 10); |
There was a problem hiding this comment.
Please bounds-check pos (aka offset) param, at least with an assert()
| static void textslider_render(component *c) { | ||
| static void render_text(component *c) { |
There was a problem hiding this comment.
Please rename this method to something that does not contain the word "render."
textslider_format? textslider_updatestr?
EDIT: probably better to remove the textslider changes from this PR in favor of PR 773.
This needs somewhat careful review, especially the string parser changes.