Way too tired in the morning, I meant as *ami* said on phab. :) On 29/09/15 08:50, Tom Hacohen wrote: > He he, yeah, client is still annoying. ;) > Thanks for taking care of everything. If it's what I think you are talking > about, the casting is a very bad idea (as you said yourself on phab). If > the function expects a non cost value, it implies you should strdup it. > > -- > Tom > On 29 Sep 2015 07:05, "SHILPA ONKAR SINGH" <[email protected]> wrote: > >> >> >> ------- Original Message ------- >> Sender : Amitesh Singh<[email protected]> >> Date : Sep 29, 2015 02:03 (GMT+09:00) >> Title : Re: [E-devel] [EGIT] [core/efl] master 17/20: eina_tmpstr: add >> eina_tmpstr_strftime >> >> Hi, >> >> >> On Mon, Sep 28, 2015 at 7:35 PM, Tom Hacohen wrote: >> >>> On 28/09/15 09:53, Tom Hacohen wrote: >>>> On 24/09/15 18:53, Cedric BAIL wrote: >>>>> On Thu, Sep 24, 2015 at 10:20 AM, Tom Hacohen >>> wrote: >>>>>> On 23/09/15 19:56, Cedric BAIL wrote: >>>>>>> On Wed, Sep 23, 2015 at 1:18 AM, Tom Hacohen >>> wrote: >>>>>>>> On 22/09/15 18:31, Cedric BAIL wrote: >>>>>>>>> Le 22 sept. 2015 09:40, "Tom Hacohen" a >>> écrit : >>>>>>>>>> >>>>>>>>>> On 22/09/15 17:32, Cedric BAIL wrote: >>>>>>>>>>> Le 22 sept. 2015 02:30, "Daniel Kolesa" a >>> écrit : >>>>>>>>>>>> >>>>>>>>>>>> On Mon, Sep 21, 2015 at 11:24 PM, Shilpa Singh < >>>>>>>>> [email protected]> >>>>>>>>>>> wrote: >>>>>>>>>>>>> cedric pushed a commit to branch master. >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>> >>> >> http://git.enlightenment.org/core/efl.git/commit/?id=abaf29cb768375957c9ee0b64d36034c21c618ea >>>>>>>>>>>>> >>>>>>>>>>>>> commit abaf29cb768375957c9ee0b64d36034c21c618ea >>>>>>>>>>>>> Author: Shilpa Singh >>>>>>>>>>>>> Date: Mon Sep 21 23:48:16 2015 +0200 >>>>>>>>>>>>> >>>>>>>>>>>>> eina_tmpstr: add eina_tmpstr_strftime >>>>>>>>>>>> >>>>>>>>>>>> This API seems awfully arbitrary and I don't really see the >>> point. >>>>>>>>>>>> Might as well be any other function that prints to strings - >> are >>> you >>>>>>>>>>>> gonna add these too? Sounds horrible to me. >>>>>>>>>>> >>>>>>>>>>> Is it better to have our code cluttered by static buffer and no >>> overflow >>>>>>>>>>> check ? Obviously not. Yes,I expect we refactor all our buffer >>> print >>>>>>>>> code >>>>>>>>>>> as it is pretty bad right now and may even lead to security >> issue >>> in >>>>>>>>> some >>>>>>>>>>> case. >>>>>>>>>> >>>>>>>>>> Just to chime in, I think this doomsday scenario is a bit >>> exaggerated. >>>>>>>>>> I think having code like: >>>>>>>>>> >>>>>>>>>> char buf[SOME_LEN]; >>>>>>>>>> strftime(...buf...); >>>>>>>>>> return tmpstr_add(buf); >>>>>>>>>> >>>>>>>>>> Is a much cleaner solution than adding all of the string >> functions >>> of >>>>>>>>>> the world to tmpstr, strbuf, stringshare and etc. >>>>>>>>>> >>>>>>>>>> I really don't like code duplication, and I think that's where >>> Daniel is >>>>>>>>>> coming from. >>>>>>>>>> >>>>>>>>>> What do you think? >>>>>>>>> >>>>>>>>> It's a good example that won't always work and increase at best >> the >>> stack >>>>>>>>> size requirement for absolutely no good reason. >>>>>>>> >>>>>>>> Why wouldn't it always work? >>>>>>>> Also if you enclose it in its own sub {}, every compiler will treat >>> the >>>>>>>> stack sanely, and even without it, I'd assume optimising compilers >>> will. >>>>>>> >>>>>>> The previous implementation used in Elementary was not working with >>>>>>> some local where it used way more charactere than expected. Work >>>>>>> around solution is to be over generous on memory... >>>>>>> >>>>>>> But compiler have no way to optimize it at all. How could they know >>>>>>> before jumping to a function call, that is going to generate data at >>>>>>> runtime, how much that function is going to use to limit the size of >>>>>>> the buffer on the stack they give to it. No compiler can optimize >>>>>>> that. It's just impossible. Every time we put a 4K buffer on the >> stack >>>>>>> all further function call will force that page allocation, that's >> how >>>>>>> things are. >>>>>> >>>>>> As implied by my comment about the own sub {}, I was talking about >>>>>> optimising the over-stack-usage. >>>>>> >>>>>> char buf[SOMELEN]; >>>>>> strftime(buf); >>>>>> ret = tmpstr_add(buf); >>>>>> // buf is not used anymore, stack can be cleared. >>>>>> >>>>>> That's what I meant. It's true though that compilers can't assume we >>>>>> haven't kept the buf pointer and used it somewhere else (which we are >>>>>> allowed to until the function ended). Though that's not what you were >>>>>> talking about. >>>>> >>>>> That is another case that a compiler can not optimize. It has no clue >>>>> that buf is not stored somewhere globally, or that ret doesn't point >>>>> to somewhere in it. That's why stack size vary depending on the block >>>>> you are in. Also stack never shrink, so once you get those 4K on it, >>>>> they will stay there. >>>> >>>> As I said, this case is unlikely, but the other case I mentioned does >>>> allow the compiler to optimise: >>>> >>>> int foo(void) >>>> { >>>> const char *bla; >>>> { >>>> char buf[LONGBUF]; >>>> strftime(...); >>>> bla = eina_tmpstr_add; >>>> } >>>> // rest of code >>>> printf("%s\n", bla); >>>> return 0; >>>> } >>>> >>>> this can even be simplified into a macro that accepts a parameter >> saying >>>> which function to use to duplicate the string. strdup, eina_tmpstr_add, >>>> eina_stringshare_add or whatever. >>>> >>>>> >>>>>> However, looking at the tmpstr code, the solution there is no better. >>> It >>>>>> reallocs all the time and implements a whole mechanism there. >> Wouldn't >>>>>> it be much better to create a eina_strftime_alloc (bad name) that >>>>>> returns an allocated string of the right size and that can be used in >>>>>> strbuf (strbuf_string_steal), tmpstr (add a string steal) and etc? >>>>>> That's writing the function once, and having one API for all of eina, >>>>>> instead of the proposed method which will add plenty and duplicate >> all >>>>>> around. >>>>> >>>>> strftime doesn't tell you the size of the string you need. It only >>>>> tells you if it failed to put the generated string inside the buffer >>>>> you gave it. There is no way around doing a realloc loop with that >>>>> API. >>>>> >>>>> As for writing the function once, it is the case right now. If I was >>>>> to add that function to strbuf or stringshare, then I would obviously >>>>> find a way to share the code. If not by just calling the tmpstr >>>>> function. In general for Eina, we do not add a function until we have >>>>> at least 2 users in our code base. So for now, I do not plan to add >>>>> strftime to any other part of Eina, as we do not have the need for it. >>>> >>>> You are adding eina_tmpstr_add() without any users in the code base... >>>> Anyhow, I think it would be better to add API in a common place, as we >>>> know it'll be used more, than to add a specific API and then >> duplicating >>>> it to a common place. >>>> >>>>> >>>>>>>>> I see no reason to have bad code like that just for fewer line of >>> code. >>>>>>>> >>>>>>>> It's not a fewer lines of code, it's fewer API and a lot less lines >>> of >>>>>>>> code. Also, adding such API implies to developers that they should >>>>>>>> expect such API, so you are just feeding the evil loop. >>>>>>> >>>>>>> And they should use it, as the way they usually manually do it lead >> to >>>>>>> bug. This is actually reducing our code base and also reduce our >>>>>>> number of actual bugs. >>>>>> >>>>>> Have a general purpose function that does that (as above), no manual >>>>>> work. Still reduces the codebase, much cleaner. >>>>> >>>>> eina_strftime is I guess what you are trying to say here. Basically >>>>> not making it a tmpstr and just use free to get rid of it. I guess >>>>> that's an acceptable solution. >>>> >>>> Yes, it's exactly what I'm saying. Let's revert this patch and work on >>>> an eina_strftime instead. >>>> >>>>> >>>>>>>> Btw, I was thinking about the whole tmpstr solution (which you know >>> I am >>>>>>>> not a fan off) last night, and I came up with two solutions I think >>> are >>>>>>>> better, and anyhow, are massively more efficient. >>>>>>>> The first involves creating new API (but it's still less API than >>>>>>>> tmpstr) for genlist and etc that accept a struct instead of a >> string. >>>>>>>> This way we can add more info that will hint who should free it. >>>>>>>> This is the more involved, clean solution, that we should maybe >> adopt >>>>>>>> for EFL 2.0. >>>>>>> >>>>>>> So your solution is to pass a struct where there will be one byte >>>>>>> dedicated to say if that string need to be freed ? I think that's >> far >>>>>>> from elegant and fully error prone with no way for the compiler to >>>>>>> help on that. >>>>>> >>>>>> Maybe we'll have more metadata in the future. The struct will be >>> created >>>>>> automatically with an helper function, so not error prone at all. >>>>> >>>>> I will have to see it, but I have yet to see what would all those >>>>> metadata be. For the moment, I don't see how this could be an >>>>> improvement over current code base. >>>> >>>> I don't think eina_tmpstr is a good solution that will scale nicely, >>>> these are just alternative ideas. >>>> >>>>> >>>>>>>> The easy hacky solution, which is a replacement to tmpstr (which is >>> also >>>>>>>> a hack) and is much simpler and more efficient, is to use this >>> scheme: >>>>>>>> For static strings you just return them: >>>>>>>> return "foo"; >>>>>>>> for dynamic you return this: >>>>>>>> return eina_tomstr_add("bar"); >>>>>>>> >>>>>>>> You can free both with eina_tomstr_free(buf); >>>>>>>> >>>>>>>> The implementation is where the magic is at, in memory, it looks >>> like this: >>>>>>>> "\x02bar", the first char being the STX (start of text) char. We >> just >>>>>>>> use eina_tomstr_str_get(buf) whenever we want to use it. It just >>> returns >>>>>>>> "buf + 1" if the first char is STX. It's infinitely faster and more >>>>>>>> memory efficient than tmpstr, I think also much cleaner. The only >> bad >>>>>>>> thing is that you need to use the str function, but I think that's >> a >>>>>>>> reasonable compromise. >>>>>>> >>>>>>> That would obviously lead to random bug. If you free a buf that has >>>>>>> not been allocated by your tomstr, it will lead to an off by one >>>>>>> access, if you are unlucky you will fall on a non allocated page. If >>>>>>> you are really unlucky it will fall on an existing data that does >> have >>>>>>> the expected value and call free on a static buffer. Also it won't >> be >>>>>>> that much faster or slower as the current implementation walk a >> short >>>>>>> list that should definitively be only a few elements long. >>>>>> >>>>>> How so? As I said, genlist gets "\x02bar", so genlist can safely >> check >>>>>> the *first* (not -1) character of the string. It can then pass the >>>>>> result of string_get() to where it needs actual text, but it keeps >> the >>>>>> real reference for future processing. I don't see why or how we'll >> have >>>>>> text that starts with "\0x02". If we want to make it even more rare, >> we >>>>>> can use two bytes. Still much more efficient than tmpstr. Why would >> the >>>>>> the current implementation be a short list? You want this to be used >>> all >>>>>> around with dynamic strings, this list will grow as usage will grow. >>>>> >>>>> Oh, so you never let the API user know that it is indeed a string and >>>>> allways make it a hidden structure. That would make the code base >>>>> quite hugly with all those str_get arround for little benefit. Also >>>>> having the risk to have a confused API with random behavior sounds bad >>>>> to me. If you want to hide information in a less problematic way, you >>>>> can hide easily one bit in the pointer itself. As your API wouldn't >>>>> autorize the use of the string directly anyway, having the least >>>>> significant bit used to say if the pointer is to be freed or not will >>>>> be clearly less error prone. Still it's hugly as hell and I think >>>>> should be avoided in Efl code base (That's not the first time we have >>>>> ruled against using that trick in Eina code base, and I am still >>>>> against this kind of trick). >>>> >>>> I think the proposed mechanism is much safer than encoding in the >>>> pointer. Encoding in the pointer will result in bad memory access and >>>> thus a crash. My suggestion can cause some issues with comparison, >>>> that's it. You can create mechanisms that make it completely error >> free. >>>> For example, you could save it internally (when you do) as a type that >>>> can't be cast to a string, and then you'd have to use the functions >>>> otherwise it wouldn't compile. This is just a clean way to maintain the >>>> string ABI but still making it safe internally. >>>> >>>>> >>>>> As for why it should stay a short list it is due to what tmpstr is >>>>> used for. Code pattern for it is of the form : >>>>> >>>>> tmp = function_returning_tmpstr(); >>>>> function_call(tmp); >>>>> tmpstr_del(tmp); >>>>> >>>>> So either we have a massive recursion (which is wrong and need to be >>>>> fixed), or we are leaking tmpstr. tmpstr is not intended to be used >>>>> for outside of a block and should not be in a structure for example. >>>>> As I said, tmpstr alive at any point in time should be pretty low and >>>>> walking that said list never be a performance issue. >>>>> >>>> >>>> True. If we only use it in genlist (and similar), this is the usage >>>> pattern and it won't be the end of the world. However, as I said, I >>>> proposed above a solution I find more clean and more general purpose. >>>> Can be used in many places and saved for the long term. This is a bit >>>> diverging from the purpose of this thread though (my bad). It's more >>>> important that we deal with the issue at hand before we freeze. >>>> >>>> >>>> >>>> >>>> Conclusion: let's add eina_strftime and eina_tmpstr_manage_new and >>>> revert this before the freeze. >>> >>> Btw, the rewrite should also fix the warnings. >>> >>> src/lib/elm_calendar.c:175:11: warning: return discards ‘const’ >>> qualifier from pointer target type [-Wdiscarded-qualifiers] >>> return eina_tmpstr_strftime(E_("%B %Y"), selected_time); >>> ^ >>> >> >> There was a quick patch to fix the warnings. But its incorrect. :/ >> https://phab.enlightenment.org/D3113 >> >> @tasn, @cedric >> I will add eina_strftime and eina_tmpstr_manage_new as discussed and >> accordingly revert my patch >> https://phab.enlightenment.org/D3048 >> will update: https://phab.enlightenment.org/D3087(example) >> and then update elm_calendar code accordingly. >> The above warning patch, I did not have a choice but to typecast due to >> signature of format_func, but hopefully with above changes, >> I wont have to typecast I will update with corrected code. >> @tasn, sorry to use same email client, will change soon >> >> >>> -- >>> Tom. >>> >>> >>> >>> >> ------------------------------------------------------------------------------ >>> _______________________________________________ >>> enlightenment-devel mailing list >>> [email protected] >>> https://lists.sourceforge.net/lists/listinfo/enlightenment-devel >>> >> >> ------------------------------------------------------------------------------ >> _______________________________________________ >> enlightenment-devel mailing list >> [email protected] >> https://lists.sourceforge.net/lists/listinfo/enlightenment-devel >> >> ------------------------------------------------------------------------------ >> _______________________________________________ >> enlightenment-devel mailing list >> [email protected] >> https://lists.sourceforge.net/lists/listinfo/enlightenment-devel >> > ------------------------------------------------------------------------------ > _______________________________________________ > enlightenment-devel mailing list > [email protected] > https://lists.sourceforge.net/lists/listinfo/enlightenment-devel > *
------------------------------------------------------------------------------ _______________________________________________ enlightenment-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
