Call and return by reference in CSFML

[rogerlevy]

I'm using a language (Forth) that has no clue about passing structs by value - it was a chore enough to create working bindings for SFML , then I had to convert the arguments to pass-by-reference for it to work properly and not have to worry if the structs change in the future.  I was wondering if it might hurt to make these (few?) functions to all use call-by-reference?  I know it seems silly to do it just for one person but I like SFML and if I ever update to 2.0 (currently using 1.6) then I'm just concerned I might have to modify the source heavily.  I know that using call-by-value is faster but so far the routines I converted like sfWindow_Create weren't performance-critical.  So what do you say?  Can you accomodate me?  I could contribute my modified source to SVN...

[Groogy]

I might misunderstand but you want to write bindings for Forth?

Anyway I wrote the bindings for Ruby and it's the same, every variable is a reference to an object and nothing is passed by value. What I did was to allocate everything on the heap whenever I wanted an object from SFML and it was fixed.

Example:

Code: [Select]


window = SFML::Window.new
The code in C/C++ run here is actually:

Code: [Select]

static VALUE Window_New( int argc, VALUE *args, VALUE aKlass )
{
sf::Window *object = new sf::Window(); // <- This is the interesting part
VALUE rbData = Data_Wrap_Struct( aKlass, 0, Window_Free, object );
rb_obj_call_init( rbData, argc, args );
return rbData;
}

Why it would be a bad idea to make everything use reference(in C that's simply pointers) is because it's pointers. We want to avoid that as it's more error-prone.

[Laurent]

You mean that Forth can import and use C libraries, but only support primitive types (numbers and pointers) in C function calls? This is pretty weird. It makes the language incompatible with many C libraries.

Anyway, I'm sorry but I'm not going to make the C API a little more complicated to use, just for the sake of having a Forth binding.

Groogy > I assume he's talking about functions arguments, not objects instances.

[Groogy]

Groogy > I assume he's talking about functions arguments, not objects instances.

Then it is more something like:

Code: [Select]


sf::Drawable *drawable = NULL;
Data_Get_Struct( args[0], sf::Drawable, drawable );
window->Draw( *drawable );


The previous does solve it, if we allocate everything on the heap for the binding then automatically we have to work with everything as references. And when we pass them on to SFML we just dereference them.

But well, I might be all wrong and misunderstood what he meant.

[Laurent]

I think he's talking about this kind of function

Code: [Select]

CSFML_API sfWindow* sfWindow_Create(
    sfVideoMode Mode, // pass by value
    const char* Title,
    unsigned long Style,
    sfWindowSettings Params // pass by value
);

You have no problem with Ruby because you write the binding in C++. I assume he's writing it directly in Forth, and thus is limited on its calls to C functions.

But I might as well completely misunderstood

[rogerlevy]

Laurent, that's right, I only have the ability to use the C functions in Forth.  

I've already written about 80% of the bindings, that isn't the problem at all because they can be fairly easily converted from the .H files.  Passing by value when the structure is more than 32-bits is problematic because the DLL-interface assumes that most parameters are exactly 32-bits. (To do 64-bits, I have to literally put two parameters instead of one.)

In theory, I could cook up a way to automatically tell Forth to allocate space for the structs on the parameter stack, but that would be extremely awkward and not reliable.  Forth is simpler than C in the backend (to the advantage of lightning-quick compilation.) so it isn't really strange I'd say, just ... primitive.

[Laurent]

Would you be able to quickly build a list of all the functions that I would have to change? Maybe I can take a look at it

[rogerlevy]

Sure, let's see ... I don't think it'll be that many but I'll take a look now!  Thanks!

[rogerlevy]

Here - just copy and pasted from the header files, hope this is OK:

CSFML_API sfWindow* sfWindow_Create(sfVideoMode Mode, const char* Title, unsigned long Style, sfWindowSettings Params);
CSFML_API sfWindow* sfWindow_CreateFromHandle(sfWindowHandle Handle, sfWindowSettings Params);
CSFML_API sfWindowSettings sfWindow_GetSettings(sfWindow* Window)
CSFML_API sfVideoMode sfVideoMode_GetMode(size_t Index);
CSFML_API sfVideoMode sfVideoMode_GetDesktopMode();
CSFML_API void sfView_SetFromRect(sfView* View, sfFloatRect ViewRect);
CSFML_API sfView* sfView_CreateFromRect(sfFloatRect Rect);
CSFML_API void sfSprite_SetSubRect(sfSprite* Sprite, sfIntRect SubRect);
CSFML_API sfRenderWindow* sfRenderWindow_Create(sfVideoMode Mode, const char* Title, unsigned long Style, sfWindowSettings Params);
CSFML_API sfRenderWindow* sfRenderWindow_CreateFromHandle(sfWindowHandle Handle, sfWindowSettings Params);
CSFML_API sfWindowSettings sfRenderWindow_GetSettings(sfRenderWindow* RenderWindow);

I already modified a few of these, here's my code:

Code: [Select]



sfWindow* sfWindow_Create(sfVideoMode* Mode, const char* Title, unsigned long Style, sfWindowSettings* Params)
{
    // Convert video mode
    sf::VideoMode VideoMode(Mode->Width, Mode->Height, Mode->BitsPerPixel);

    // Create the window
    sfWindow* Window = new sfWindow;
    sf::WindowSettings Settings(Params->DepthBits, Params->StencilBits, Params->AntialiasingLevel);
    Window->This.Create(VideoMode, Title, Style, Settings);
    Window->Input.This = &Window->This.GetInput();

    return Window;
}


sfWindow* sfWindow_CreateFromHandle(sfWindowHandle Handle, sfWindowSettings* Params)
{
    sfWindow* Window = new sfWindow;
    sf::WindowSettings Settings(Params->DepthBits, Params->StencilBits, Params->AntialiasingLevel);
    Window->This.Create(Handle, Settings);
    Window->Input.This = &Window->This.GetInput();

    return Window;
}



Code: [Select]



sfVideoMode Ret;

////////////////////////////////////////////////////////////
/// Get a valid video mode
/// Index must be in range [0, GetModesCount()[
/// Modes are sorted from best to worst
////////////////////////////////////////////////////////////
sfVideoMode* sfVideoMode_GetMode(size_t Index)
{
    sf::VideoMode Mode = sf::VideoMode::GetMode(Index);    
    Ret.Width        = Mode.Width;
    Ret.Height       = Mode.Height;
    Ret.BitsPerPixel = Mode.BitsPerPixel;
    return &Ret;
}


[Laurent]

Ok thanks

Some of them would be better with pointers (like sfWindowSettings -- this way we can pass NULL to get the default behaviour).

I'm more concerned about return values though. Declaring a global variable for each function that returns something is... ugly (and not multithread-friendly). Moreover you get a pointer to something internal, you don't know if you can modify it, or if it may be modified later by SFML, etc. Not clean at all.

[rogerlevy]

Yeah, agreed ...

Something I like to use often in this scenario, when I want to pass back something that I want to be unique and modifiable, but not have to be responsible for freeing it myself, is circular buffers.  An array of 8 or 16 of whatever object and an index that is used to get the next object, and moves circularly through the array based on its length, is more or less acceptable.  In C it might be a little awkward ... Forth makes it easy to add as a language feature. X)

Only way Forth can really use objects passed back is to have an address to something *not* on the stack, otherwise it will basically get clobbered almost immediately.  Maybe a workaround, like a small wrapper DLL...

[Laurent]

Maybe a workaround, like a small wrapper DLL...

Could be a solution, yes.

[rogerlevy]

I guess, in places where it would be beneficial, you could make the parameter changes to CSFML ...

Anyway, aren't return-by-value structs still not so great, because, if you want the caller to return that struct, won't it go out of scope 2 levels up?

Be nice if those functions would just take an address to a struct to fill like I've seen in some other functions, and be done with it.

[Laurent]

Anyway, aren't return-by-value structs still not so great, because, if you want the caller to return that struct, won't it go out of scope 2 levels up?

Sorry, I don't understand what you mean. Return by value is the best option: since you never "point" to something that exists somewhere else, you can never go out of scope. You are the owner of what you get after calling the function.

Be nice if those functions would just take an address to a struct to fill like I've seen in some other functions, and be done with it.

It's a huge source of errors, and it's more verbose in my opinion.
I often see people doing this kind of error:

Code: [Select]

sfWindowSettings* settings;
sfWindow_GetSettings(window, settings); // crash

[rogerlevy]

On second thought, I realized that was completely wrong-sauce, you're right.  Sorry.  The structs get copied around so you don't lose anything I believe.

On the second point ... yeahhh ... seem to remember making similar mistakes.  Is there a better alternative?