On 23/09/15 19:56, Cedric BAIL wrote:
> 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.
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.
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.
>
>>> 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.
>
>> 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.
>
>> 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.
>
>> 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.
>
Trust me on that, I think less of tmpstr than you think on any of these
proposed solution. However, I don't say things like "you made me laugh",
show some class, and behave in a reasonable manner. You are a long time
EFL developer, please orchestrate yourself in the same way you expect
others to.
The issue is *not* the tmpstr part of the API, the issue was, as
mentioned above, the duplication. Even with both of my proposed
solutions you could use the same argument that strftime functions are
needed, so "attacking" strftime doesn't serve any anti-tmpstr goals.
Furthermore, speaking of tmpstr, I'm annoyed it's still in, yes,
especially since when it was discussed on the ML the consensus was that
it sucks and shouldn't get in (Adrien, Kolesa and myself objecting, you
calling us idiots and not responding at some point). We don't follow up
on discussions enough, and that's what happens, things stay in. It
should have been discussed in advance, but whatever.
Just to say it again though, this thread is not about tmpstr, it's about
the duplication. I mentioned the above methods as an afterthought.
--
Tom.
------------------------------------------------------------------------------
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
