Page MenuHomePhabricator

evas_textblock_edje: replace fixed size array in reparse
Needs RevisionPublic

Authored by ali.alzyod on Aug 28 2019, 5:35 AM.

Details

Summary

replace fixed size array, with dynamic size array to work with font length exceeds 120

Diff Detail

Repository
rEFL core/efl
Branch
arcpatch-D9769
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 13020
Build 9271: arc lint + arc unit
ali.alzyod created this revision.Aug 28 2019, 5:35 AM

It seems that this patch has no reviewers specified. If you are unsure who can review your patch, please check this wiki page and see if anyone can be added: https://phab.enlightenment.org/w/maintainers_reviewers/

ali.alzyod requested review of this revision.Aug 28 2019, 5:35 AM
smohanty added inline comments.Aug 28 2019, 5:50 AM
src/lib/edje/edje_textblock_styles.c
123

well the static buffer was here for a reason(to avoid memory allocation) . so if you are paranoid that the font name can be bigger than 100 character , then use the malloc by doing a strlen() check if its exceeds the buffer, otherwise use the static buffer.

smohanty requested changes to this revision.Aug 28 2019, 5:51 AM
This revision now requires changes to proceed.Aug 28 2019, 5:51 AM
ali.alzyod updated this revision to Diff 24559.Aug 28 2019, 6:52 AM

add stack buffer

smohanty added inline comments.Aug 28 2019, 12:31 PM
src/lib/edje/edje_textblock_styles.c
122

good , now just wrap this block of code in some small function like

const char*
_embedded_font(const char* font_name);

or pick your function name :)

then this code block will be replaced with

tag_ret->font = _embedded_font(val);

smohanty requested changes to this revision.Aug 28 2019, 12:31 PM
This revision now requires changes to proceed.Aug 28 2019, 12:31 PM
ali.alzyod added inline comments.Aug 29 2019, 1:04 AM
src/lib/edje/edje_textblock_styles.c
122

I will need to create other function to clear the buffer too, so do you think it worth to create two functions to replace this code ?

ali.alzyod marked an inline comment as done.Aug 29 2019, 1:19 AM
smohanty accepted this revision.Aug 29 2019, 2:31 AM
This revision is now accepted and ready to land.Aug 29 2019, 2:31 AM

../src/lib/edje/edje_textblock_styles.c: In function ‘_embedded_font’:
../src/lib/edje/edje_textblock_styles.c:72:13: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]

ret = eina_stringshare_add(buffer);
    ^

../src/lib/edje/edje_textblock_styles.c: In function ‘_edje_format_reparse’:
../src/lib/edje/edje_textblock_styles.c:122:60: warning: passing argument 1 of ‘_embedded_font’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]

tag_ret->font = _embedded_font(val);
                               ^~~

../src/lib/edje/edje_textblock_styles.c:55:15: note: expected ‘char *’ but argument is of type ‘const char *’
static char * _embedded_font(char * font)

please fix upon warning messages.

SanghyeonLee requested changes to this revision.Aug 29 2019, 2:41 AM
This revision now requires changes to proceed.Aug 29 2019, 2:41 AM
smohanty added inline comments.Aug 29 2019, 5:46 AM
src/lib/edje/edje_textblock_styles.c
55

change it to

static const char* _embedded_font(const char *font)

fix warnings

bu5hm4n requested changes to this revision.Nov 5 2019, 8:42 AM
bu5hm4n added a subscriber: bu5hm4n.

I am not too sure if the intent to use malloc if the size is bigger than 100 is a good idea at all. why not just use alloca ? There is eina_stringshare_nprintf which is normally used for such purposes. And i think it makes a lot of sense to use this here as well. If there is a issue with > 100 characters on the stack, then we simply can resolve this as well in the eina_stringshare implementation and fix this as well for all the other cases.

This revision now requires changes to proceed.Nov 5 2019, 8:42 AM

I am not too sure if the intent to use malloc if the size is bigger than 100 is a good idea at all. why not just use alloca ? There is eina_stringshare_nprintf which is normally used for such purposes. And i think it makes a lot of sense to use this here as well. If there is a issue with > 100 characters on the stack, then we simply can resolve this as well in the eina_stringshare implementation and fix this as well for all the other cases.

alloca is not good if you do not know the exact size, because it can cause stack overflow

Independent from stack overflow or not, also solve the issue
there, my main goal about this here is to reduce code duplication, and this is exactly heading towards duplication...

cedric requested changes to this revision.Nov 5 2019, 12:22 PM
cedric added inline comments.
src/lib/edje/edje_textblock_styles.c
78

Just use an Eina_Strbuf! If you want to optimize the memory allocation, you can even use eina_strbuf_manage_read_only_new_length with your buffer_stack. Be careful with complex code that manipulate string, this are hard to get right and not smack things around.