Code::Blocks Forums

Developer forums (C::B DEVELOPMENT STRICTLY!) => Development => Topic started by: frithjofh on May 21, 2017, 07:25:38 pm

Title: small addition to editormanager - for convenience only
Post by: frithjofh on May 21, 2017, 07:25:38 pm

hi everybody,

i just added a convenience function to EditorManager class. it is purely thought to unclutter so many for loops where one first gets the count of editors, loop over them with an index, then tests if they are built in editors or null for some other reason, then finally use them. with this function, one can use the new for ( x : container ) loop syntax, and directly use the editor* and save many lines of code.

the other thing was changing the signiture of:

int GetEditorsCount()

to

size_t GetEditorsCount() const

as the underlying function returns this type anyways, and making the function const. doesn't break anything. as always for the devs to kindly consider

regards

frithjofh

Title: Re: small addition to editormanager - for convenience only
Post by: oBFusCATed on May 22, 2017, 08:59:28 am

Please don't use size_t or any other unsigned type for such things.
They cause more problems than they fix!
Also this patch probably breaks the API and the ABI, so your statement at the end is incorrect. :)

Title: Re: small addition to editormanager - for convenience only
Post by: frithjofh on May 22, 2017, 09:31:55 am

thx for the reply.

i can't see how this patch could break the API and ABI. the original function goes like this:

Code

int EditorManager::GetEditorsCount()
{
    return m_pNotebook->GetPageCount();
}

where the underlying function is defined as so:

Code

    size_t wxAuiNotebook::GetPageCount() const;

so in its present state the function already returns a size_t.

or do you refer to the const breaking any existing code?  ???

it does compile without any errors, warnings or misbehavior on my machine...




and the other change? adding the function returning a vector of the built in editors?

Title: Re: small addition to editormanager - for convenience only
Post by: oBFusCATed on May 22, 2017, 06:03:14 pm

I have not looked at the patch, but changing the signature of a function affects both the API and the ABI!
We don't care too much about both, but I wanted to mention it, so you are aware.

Title: Re: small addition to editormanager - for convenience only
Post by: frithjofh on May 22, 2017, 06:24:42 pm

ok. i really am aware, but thx anyways. as i am autodidact, i really appreciate any advice or tip  :)

and yes, the change made led to the recompilation of the core and a number of plugins, took about 4 min on my machine to finish without extra warnings or even errors

Title: Re: small addition to editormanager - for convenience only
Post by: oBFusCATed on May 22, 2017, 09:41:52 pm

This patch looks unacceptable to me. I don't like that every time GetBuiltinEditors is called one new vector is allocated.
For me this is a wasteful api that might be hard to change in the future if it turns out that this allocation is hurting performance.
If you want to do it you need to use some kind of an accessor class that implements begin/end and probably some kind of iterators.

Title: Re: small addition to editormanager - for convenience only
Post by: frithjofh on May 22, 2017, 10:05:01 pm

i don’t think there is any big performance hit, if any. consider it being used in a for ( x : GetBuiltinEditors() ) loop. the vector of pointers will be just here as temporary moved out of the function on return.

it is not a function that gets called many times and its share in execution time is negligible compared to the things that happen later in the function actually working on the pointers to the editors.

in the end it is not a bottleneck function anywhere in the code.

if it does hurt performance, any programmer is free to not use it and stay with the simpler functions, as of now

in places where performance is not the issue, or if it does not actually imply worse performance it will definitely be a more readable and less error prone way to express what the programmer wants.

but be it as it may. your the devs, you decide...  :)

Title: Re: small addition to editormanager - for convenience only
Post by: frithjofh on May 28, 2017, 05:17:53 pm

is this question decided already?

Title: Re: small addition to editormanager - for convenience only
Post by: oBFusCATed on May 28, 2017, 06:11:34 pm

Have you provided a better patch that fixes my concerns?
At the moment the only acceptable change I see is the addition of a const to one of the methods and it is partly acceptable, because it breaks the ABI.

Title: Re: small addition to editormanager - for convenience only
Post by: frithjofh on May 28, 2017, 07:09:46 pm

well. honestly, i think that you are perhaps overrating your concerns, no offense meant.

in the first place, the new function i provide is only a convenience function. so it doesn't harm performance, where performance is needed. where performance is needed, any concerned programmer can and will choose the underlying, and claimed to be faster, functions already in the API.

in the second place, if one takes a look at the typical function using this API functions, it goes more or less like this:

1) first it sets up a normal loop, calling on every iteration the GetEditorCount() function
2) the index is converted into a pointer to a built in editor by calling GetBuildinEditor() or similar
3) the pointer is checked against nullptr
4) then something is asked of the pointer like for instance GetName()
5) then that name is most likely put into an array or added to some wxwidget

in my (humble) opinion, the amount of execution time spend in 4) and 5) normally totally out-ways what is done in the rest of the loop. so, using the convenience function in this case would hurt performance only in a almost unnoticeable amount. in return it would reduce significantly the lines of code and checks performed over and over again in so many places in the code, with so many possibilities to forget something, or introduce an error...

i am in no way an expert, but only the fact, that i have to read less lines to grasp what a piece of code wants to achieve seems like a huge benefit, and as i stated above, in many of those calls, the code is not so performance critical as it seems. and as to resources: how many pointer would that vector have to hold? 10, 42, 128? it doesn't seem so wastefull at all: the vector gets allocated on stack for how much time, before it goes away?

but maybe i don't get the point, if so, please just ignore my proposition

Title: Re: small addition to editormanager - for convenience only
Post by: oBFusCATed on May 28, 2017, 07:42:03 pm

I'm not against making the API easier and less error prone.
I'm against designing APIs that are slow from the beginning.
People are bad at predicting the future and past experiences have shown that performance matters almost everywhere.
One place you use a bad algorithm or data structure and sometime in the future it shows in the profile.

If you make an adapter or generator like class that has support for range-based-for, I'll be happy to push it.

Title: Re: small addition to editormanager - for convenience only
Post by: frithjofh on May 28, 2017, 07:54:02 pm

i actually had something like implemented, but ir even felt more intrusive. give me some days and i will shape it up and present it to you.

basically it was a class just forwarding the according calls to the existing functions, all awhile providing means to be pkaced in a for loop or even a stl algorithm. with begin() and end() and all that. some static functions and little more

at the time i discarded it because it seemed overkill...