Design and documentation

[nullsquared]

Nice. Not knowing elementary optimization techniques, but laughing at my argument. :roll:

Google turns up nothing useful for "RVO", therefore you are using some uncommon term that perhaps only you and a handful of other people use.  Instead of explaining it, you try to make me look stupid.  Who's rolling their eyes now?

I already posted them at the beginning. I even repeated them.

If your only argument is because "it goes against SFML's philosophy" then OK.

[Nexus]

Google turns up nothing useful for "RVO", therefore you are using some uncommon term that perhaps only you and a handful of other people use.

Ehm, not really. This term is quite spread in the C++ community. If you type "C++ RVO" at Google, then already the first link explains what the abbreviation stands for. Besides, there is a corresponding wikipedia entry.
 

Instead of explaining it, you try to make me look stupid.  Who's rolling their eyes now?

Who began laughing at justified arguments? Anyway, let's keep a meaningful discussion now.

If your only argument is because "it goes against SFML's philosophy" then OK.

No, just read my posts. I really try to bring comprehensible arguments. So I'd appreciate you attempting to understand my point of view.

When you use pointers instead of references to show that an object is not copied, this problem may disappear, but in contrast, the interface doesn't allow statements about the validity of null pointers any more. You can't have both pros directly in the code. Besides, the pointer way makes non-const-reference parameters useless.

Yes, but as stated, another problem arises. SFML uses pointers when they can be null. If one can pass a pointer, beginners may think null represents something like the default view, but this will lead to undefined behaviour.

To sum up: There are two ways to go. Two code styles. Each of them has its advantages and its drawbacks. When designing a library, a decision must be taken, and this decision must be followed through with in order to keep up a consistent design. Laurent chose the reference way, he could also have chosen the alternative. I think there is not too much to discuss.

[nullsquared]

Ehm, not really. This term is quite spread in the C++ community. If you type "C++ RVO" at Google, then already the first link explains what the abbreviation stands for. Besides, there is a corresponding wikipedia entry.

I know about return value optimization.  Haven't really seen many people abbreviate it as RVO, though.

No, just read my posts. I really try to bring comprehensible arguments. So I'd appreciate you attempting to understand my point of view.

Your restated argument is essentially exactly what I mentioned: "because it goes against SFML's philosophy."

[Nexus]

Your restated argument is essentially exactly what I mentioned: "because it goes against SFML's philosophy."

Actually, I brought C++ arguments against pointer parameters, when references match as well. This has nothing to do with SFML.

But this seems to be not okay, so what do you want to hear?

[nullsquared]

Actually, I brought C++ arguments against pointer parameters, when references match as well. This has nothing to do with SFML.

What exactly, that they can be NULL?

So throw an exception.

But you can't, because that's against SFML's philosophy.  What's your point?

[Nexus]

What exactly, that they can be NULL?

So throw an exception.

But you can't, because that's against SFML's philosophy.

No, that's not the only reason*. It is always bad to transfrom a compile-time constraint (like a reference, which cannot be null by definition) into a runtime-error. Plus, we get overhead in speed because of the additional pointer check. Both drawbacks don't exist if we use references.

______
* But it is a reason. Why don't you get along with SFML's design? If you write a library, users have to work with it as well. Of course you can discuss about it, here you have even got the possibility to talk to the developper himself, but what's the point of this specific discussion? It's a matter of taste, like naming conventions. Laurent has already explained why he doesn't like your approach and you replied "Ok."

[nullsquared]

It is always bad to transfrom a compile-time constraint (like a reference, which cannot be null by definition) into a runtime-error.

Ok.  What's your argument against a non-const reference? (which makes it obvious that you shouldn't be passing local variables)

Plus, we get overhead in speed because of the additional pointer check.

I'll consider your argument when you show me a profiler log that even mentions said check.

Why don't you get along with SFML's design?

Because I've worked with many libraries and I don't like SFML's design.  I'd write my own renderer but I don't want to deal with low level code at the moment, and the fact that SFML 2 will have VBOs is very compelling.

[Nexus]

Ok.  What's your argument against a non-const reference? (which makes it obvious that you shouldn't be passing local variables)

It doesn't take const-correctness into account. If the reference parameter isn't modified, it should be const-qualified. Otherwise, you restrict many use cases where you only have a const-qualified object.

I'll consider your argument when you show me a profiler log that even mentions said check.

Of course it isn't time-critical in the very most cases. There's still no reason to waste little time, you don't know the user's requests. Apart from this, with references, you don't have to write and maintain the checking code, either.

But anyway, I'd use assert and not exceptions for such cases.

[nullsquared]

My problem is that you're preventing people from using NULL (which is something that you can check for), but at the same time you're introducing the option to use temporaries (something you cannot check for).

This:

Code: [Select]


window.SetView(NULL);

is a much more visible error than this:

Code: [Select]


{
    sf::View view(...);
    window.SetView(view);
}

It's extremely hard to track down the second problem.

Or even worse:

Code: [Select]


window.SetView(sf::View(...));


[Laurent]

Thank you guys for your feedback, and for defending your ideas so passionately. I really appreciate it, that's what helps me to make SFML better.

I'll try to summarize what I've found useful in this discussion.

1. Using a const reference makes really unclear what the function does with the object, and something must be done to prevent people from using it wrongly. I'll for sure improve the documentation a lot, and regarding this particular RenderTarget::SetView function, I'm thinking about handling views by value rather than by keeping references to user instances.
I'm not going to use pointers because of personnal design choices, but we all agree that it's not the only solution anyway.

2. To me, mutable members and const_cast were similar solutions to the problems I have in SFML, but apparently mutable members are still cleaner and more correct. I already replaced const_cast with them wherever it was obvious in SFML 2 (like in sf::View), but maybe I should use them everywhere, even if most of the members will become mutable (sf::Image, etc.).

3. This whole discussion made me think about something that could potentially be a huge improvement to the drawable classes in SFML, it has nothing to do with the actual subject of this topic but thanks

This was not meant to close the discussion, so if someone disagrees with what I just said, or still feels like something's really wrong in SFML, just go on :wink:

[Nexus]

I'm thinking about handling views by value rather than by keeping references to user instances.

In my opinion, the reference way is okay, as it allows the user to modify the view directly, while the window adapts those modifications silently. But of course, this flexibility goes at the expense of possible wrong usage. The safest way to still keep references is not very esthetic, but a function name like SetReferencedView() might spell this issue out. Just a spontaneous idea.

[Dravere]

How often do you change the view? I don't think that a copy of around 100 Bytes would do much damage. And if so, you could perhaps split the view up a bit. Is the matrix really necessary to be in the view the user uses? Couldn't the window use some type of ViewContainer which has an additional matrix. That would make 64 Bytes less to copy

As an abstract:

Code: [Select]


class View
{
public:
  // ... the known interface ...

private:
  sf::Vector2f myCenter;
  sf::Vector2f myHalfSize;
  FloatRect    myRect;
// remove this:  Matrix3 myMatrix;
// If I'm right we don't need this anymore either: bool myNeedUpdate;
// I think even the friend RenderWindow can be removed ...
};

// ...

class RenderWindow
{
private:
  class ViewHolder
  {
  public:
    //...
    void SetView(View const& view)
    {
       myView = view;
       myNeedUpdate = true;
    }
    //...
  private:
    View myView;
    Matrix3 myMatrix;
    bool myNeedUpdate;
  };

public:
  // ...

  void SetView(View const& view)
  {
    m_viewHolder.SetView(view);
  }


private:
  ViewHolder m_viewHolder;
};

I hope you can understand what I mean

Dravere

[Laurent]

In my opinion, the reference way is okay, as it allows the user to modify the view directly, while the window adapts those modifications silently.

The more I think about it, the less I like it. I like the idea of modifying the view at a single place (SetView), and storing it by value:
- it's easier to track changes to the current view, as it can only happen with a call to SetView
- it allows more optimizations, as the view can no longer change silently outside the RenderTarget
- it allows users to use temporary/local views

How often do you change the view? I don't think that a copy of around 100 Bytes would do much damage

Absolutely, this is not a problem. I don't care about the copy operation

[nullsquared]

SetReferencedView() might actually not be a bad idea.

In the end, my point is that while you're avoiding NULL, you're not avoiding temporaries or badly scoped View's, which can be even worse since there's no way to check for that.

Better documentation will definitely also help

Laurent, I respect your attitude towards the discussion, so no disrespect meant in any way if I came off like that :lol:

As for making lots of things mutable, you don't have to.  I forgot what this technique is called, but you can do this:
(header)

Code: [Select]


class someClass
{
    private:

        // forward declare internal data type
        struct data_t;
        data_t *_data; // keep a pointer to it since we can't instantiate, yet

    public:

        someClass();
        ~someClass();

        void someConstFunc() const;
};

(cpp)

Code: [Select]


// details of data_t here, not visible in header
struct someClass::data_t
{
    int n;
};

someClass::someClass()
{
    _data = new data_t();
    _data->n = 13;
}

someClass::~someClass()
{
    delete data;
}

void someClass::someConstFunc() const
{
    data->n = 42; // legal, data itself isn't changing

    //data = differentData; // illegal, data itself is changing
}

int main()
{
    someClass inst;
    const someClass &cref = inst;
    cref.someConstFunc();
}

No mutables, no const_cast, and no visible implementation details to the user

[Dravere]

I forgot what this technique is called, ...

It is called pimpl, handle-body or compiler firewall idiom.

No mutables, no const_cast, and no visible implementation details to the user

And a lot of additional possibility for failure, like there is one in your Code. You need to implement your own copy-constructor and operator=.

I would suggest to use this idiom with care and not only because you can avoid const_cast or mutable.

Dravere