On Wed, Sep 23, 2015 at 1:18 AM, Tom Hacohen <[email protected]> wrote:
> On 22/09/15 18:31, Cedric BAIL wrote:
>> Le 22 sept. 2015 09:40, "Tom Hacohen" <[email protected]> a écrit :
>>>
>>> On 22/09/15 17:32, Cedric BAIL wrote:
>>>> Le 22 sept. 2015 02:30, "Daniel Kolesa" <[email protected]> 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 <[email protected]>
>>>>>> 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.
>> 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.
> 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.
> 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.
> Thoughts?
You made me laught ! I expected that the issue was actually the tmpstr
part of the API. It was fun to read your attempt to reinvent it in a
worse way. Hopefully one day you will come up with a better useful
solution. In the mean time, let's just continue to roll in useful
feature that fix actual bugs and improve our code base.
--
Cedric BAIL
------------------------------------------------------------------------------
Monitor Your Dynamic Infrastructure at Any Scale With Datadog!
Get real-time metrics from all of your servers, apps and tools
in one place.
SourceForge users - Click here to start your Free Trial of Datadog now!
http://pubads.g.doubleclick.net/gampad/clk?id=241902991&iu=/4140
_______________________________________________
enlightenment-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
