Design and documentation

[nullsquared]

First of all, why are so many things assigned by value (like shapes, it seems)?

And second, why are things that look like they are assigned by value, assigned by pointer?  This is what I'm talking about:

Code: [Select]


void RenderTarget::SetView(const View& NewView)
{
    myCurrentView = &NewView;
}

I wasted so much time trying to figure out why this didn't work, knowing that SetView() took a constant reference, and therefore assuming it wouldn't store my object:

Code: [Select]


sf::View view; /* use view */
myRt.SetView(view); // wtf, changes don't seem to have any effect

The next to useless documentation doesn't really help in this regard, either...

[Nexus]

First of all, why are so many things assigned by value (like shapes, it seems)?

What do you exactly mean?

And second, why are things that look like they are assigned by value, assigned by pointer?

I think, "look like" depends on your code style. 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.

The next to useless documentation doesn't really help in this regard, either...

In this point, I agree with you. A lot of descriptions in the doc don't really say more than the function name (but okay, a lot of functions needn't be further explained). However in cases like here, where the semantics aren't obvious from the context, a notice wouldn't be a bad idea.

[nullsquared]

First of all, why are so many things assigned by value (like shapes, it seems)?

What do you exactly mean?

Shape::Line(...), Shape::Rectangle(...), and so on, return a Shape by value.  I don't even see a copy constructor for Shape or Drawable, what in the world is going on?

And second, why are things that look like they are assigned by value, assigned by pointer?

I think, "look like" depends on your code style. 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.

It's quite widely accepted to use const references as parameters in functions to avoid extra copies.  For example, Drawable::SetPosition() accepts a const reference to avoid copying the vector parameter.  However, by your (or SFML's) "code style", I would have to double check this myself just in case SFML decides to store a pointer to my position, instead of updating its own internal position (which is what happens with SetView()).

There are two solutions: use a pointer (which makes it obvious that the intent is to store the parameter), or use a non-const reference (which makes it obvious that the intent is to modify the parameter, and therefore allow storing).

That's not even the only issue.  Look at this monstrosity:

Code: [Select]


const sf::FloatRect& View::GetRect() const
{
    // Recompute it if needed
    if (myNeedUpdate)
        const_cast<View*>(this)->RecomputeMatrix();

    return myRect;
}

const_cast is almost always a cause for concern about the design.  Why is the View getting passed around by const reference if it's only going to const_cast itself into a non-const View?!?

[Laurent]

Shape::Line(...), Shape::Rectangle(...), and so on, return a Shape by value. I don't even see a copy constructor for Shape or Drawable, what in the world is going on?

Of course they do. Do you suggest that they should allocate a new instance with new, and leave the caller with a pointer and the obligation to call delete?
Shape and Drawable only have trivial members, thus the default generated copy constructor is enough, I don't need to define a custom one.

It's quite widely accepted to use const references as parameters in functions to avoid extra copies. For example, Drawable::SetPosition() accepts a const reference to avoid copying the vector parameter. However, by your (or SFML's) "code style", I would have to double check this myself just in case SFML decides to store a pointer to my position, instead of updating its own internal position (which is what happens with SetView()).

I disagree. I use references to tell the caller "hey, you don't have the option to pass a NULL value that would do some special stuff, just pass a valid object". The only thing to do to make this clear is to have a good documentation. I agree that it is poor in SFML 1.x, but it will be much much better in SFML 2.

There are two solutions: use a pointer (which makes it obvious that the intent is to store the parameter), or use a non-const reference (which makes it obvious that the intent is to modify the parameter, and therefore allow storing).

Non-const reference means that the object will be modified, which is false here. When you pass a view to a render-window, you know that it won't be changed by it, because of the const reference. With a non-const reference, anything could happen and you can't assume that your view won't suddenly change.

const_cast is almost always a cause for concern about the design. Why is the View getting passed around by const reference if it's only going to const_cast itself into a non-const View?!?

In graphics programming, const_cast are not a design issue. It's an optimization technique called "lazy evaluation", ie. things are computed and updated only when you request them. In a perfect world, I would compute the view's rectangle after a call to any setter, and leave GetRect() const. But that's inefficient. I use the same technique in other classes where performances matters.

I think that the only real problem is the documentation, and like I said it will be fixed in SFML 2
I already updated the system, window and audio modules, and some classes of the graphics module.

[Nexus]

Shape::Line(...), Shape::Rectangle(...), and so on, return a Shape by value.  I don't even see a copy constructor for Shape or Drawable, what in the world is going on?

Why does a class need an explicitly defined copy constructor if the compiler-generated one fits perfectly?

It's quite widely accepted to use const references as parameters in functions to avoid extra copies.

Yes, of course. That const-references are used to avoid unnecessary copies isn't the point of this discussion. You want it to be the only use case, that's why I contradict.

However, by your (or SFML's) "code style", I would have to double check this myself just in case SFML decides to store a pointer to my position, instead of updating its own internal position (which is what happens with SetView()).

Most of the setters in SFML copy their parameters. At exception cases, a documentation comment would be appropriate, but that's it, in my opinion.

There are two solutions: use a pointer (which makes it obvious that the intent is to store the parameter)

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.

use a non-const reference (which makes it obvious that the intent is to modify the parameter, and therefore allow storing).

Ignoring const-correctness is no option. Don't make parameters modifiable, if they aren't modified. Design breaks like that are the reason why workarounds like const_cast exist.

That's not even the only issue.  Look at this monstrosity:

Code: [Select]


const sf::FloatRect& View::GetRect() const
{
    // Recompute it if needed
    if (myNeedUpdate)
        const_cast<View*>(this)->RecomputeMatrix();

    return myRect;
}

const_cast is almost always a cause for concern about the design.  Why is the View getting passed around by const reference if it's only going to const_cast itself into a non-const View?!?

You're right, the use of const_cast is questionable. It would probably be a better solution to const-qualify sf::View::RecomputeMatrix(), and make the affected members mutable (I think the matrices aren't intended be part of the user-related instance state).

[Laurent]

It would probably be a better solution to const-qualify sf::View::RecomputeMatrix(), and make the affected members mutable (I think the matrices aren't intended be part of the user-related instance state).

Making members mutable is a little cleaner, but it appears in the public headers. Plus, a lot of members would be mutable, even those that are not just internal details of the class.
const_casts, on the other hand, are uglier but stay hidden in the implementation, and I prefer that

[Nexus]

Making members mutable is a little cleaner, but it appears in the public headers.

Yes, but why is this a problem? The average user isn't interested in private variables. Or do you mean it because changes in the header force including translation units to recompile? If it's just a bad feeling, I can still understand it; I sometimes typedef types so that they fit the space.

Plus, a lot of members would be mutable, even those that are not just internal details of the class.

Taking a closer look at the sf::View class, I see that 4 members (matrices and related bool flags) are already mutable. You're probably right, there is not very much left...
Anyway, in SFML 2, there is no GetRect() anymore.

const_casts, on the other hand, are uglier but stay hidden in the implementation, and I prefer that

Ah, you're one of the stealth hackers.
No, the effect is eventually the same. I just wondered, because in my projects, I can solve almost every conflict by mutable. But okay, I haven't written a multimedia framework, maybe I would have acted equally when the mutable keywords would have become annoying. At least you don't use C-casts, nor you really violate const-correctness.

[nullsquared]

Shape::Line(...), Shape::Rectangle(...), and so on, return a Shape by value. I don't even see a copy constructor for Shape or Drawable, what in the world is going on?

Of course they do. Do you suggest that they should allocate a new instance with new, and leave the caller with a pointer and the obligation to call delete?

No, smart pointers.

Shape and Drawable only have trivial members, thus the default generated copy constructor is enough, I don't need to define a custom one.

The problem is that this copy might be expensive.  The vector of points in the Shape is copy constructed, and the non-primitive type it holds (Point, about 24 bytes) must also be copy-constructed for each instance, and I'm sure you can imagine what happens with circles or other shapes with many points.  Other than that, I was assuming VBOs were used, which would warrant some advanced copy construction.  I was wrong.

Non-const reference means that the object will be modified, which is false here. When you pass a view to a render-window, you know that it won't be changed by it, because of the const reference. With a non-const reference, anything could happen and you can't assume that your view won't suddenly change.

So use a pointer to a const View.  Throw an exception on a NULL view.

In graphics programming, const_cast are not a design issue.

const_cast is a design issue regardless of the area it is used in.

It's an optimization technique called "lazy evaluation", ie. things are computed and updated only when you request them. In a perfect world, I would compute the view's rectangle after a call to any setter, and leave GetRect() const. But that's inefficient. I use the same technique in other classes where performances matters.

I highly doubt you need lazy evaluation for a View class.  How many times do you expect the user to modify the View in a single frame?  You'll be hitting performance issues much earlier with immediate-mode rendering than calling RecomputeMatrix() 2 or 3 extra times (1 call is ~14 additions and ~4 divisions, that's nothing).

I think that the only real problem is the documentation, and like I said it will be fixed in SFML 2

OK, thanks

[Dravere]

Making members mutable is a little cleaner, but it appears in the public headers. Plus, a lot of members would be mutable, even those that are not just internal details of the class.
const_casts, on the other hand, are uglier but stay hidden in the implementation, and I prefer that :)

The problem is: It is undefined behavior.

ISO IEC 14882:2003 7.1.5.1 The cv-qualifiers Paragraph 4

Except that any class member declared mutable (7.1.1) can be modified, any attempt to modify a const object during its lifetime (3.8) results in undefined behavior.

Drave

[Nexus]

Of course they do. Do you suggest that they should allocate a new instance with new, and leave the caller with a pointer and the obligation to call delete?

No, smart pointers.

And force heap allocation plus overhead for shared semantics? No, that's no real alternative.

The problem is that this copy might be expensive.  The vector of points in the Shape is copy constructed, and the non-primitive type it holds (Point, about 24 bytes) must also be copy-constructed for each instance, and I'm sure you can imagine what happens with circles or other shapes with many points.

Don't forget RVO.

Other than that, I was assuming VBOs were used, which would warrant some advanced copy construction.  I was wrong.

Good design means to separate rendering issues and logics, so that each part is independent of the other and can be exchanged (this is even the case in SFML). Don't draw conclusions just because of the implicitly-generated copy constructor.

In general, if not every class needs to implement the Big Three, this is a good indication. The reason is, the developper spent thoughts on giving the single components value-semantics. Every class, that manages its copy semantics and memory management automatically, contributes to a stable and less error-prone library.

So use a pointer to a const View.  Throw an exception on a NULL view.

The things you demand break SFML's central design decisions. One of them is to use references when no null value is allowed, another of them is not to use exceptions. Laurent had to come to a decision and remain consistent. If the design looked the way you liked it, there were other people not being content. The topic is similar to the naming convention.

[Laurent]

No, smart pointers.

That would indeed solve the problems I mentionned. But do we really need that extra level of complexity (it may not be complex for you and me, but it is for beginners)? What problem is there with a return by value?

The problem is that this copy might be expensive

Creating a new shape is generally a one-time operation, not something that has to be done 60 times per second.

So use a pointer to a const View. Throw an exception on a NULL view.

Why would I allow something and thrown an exception when it is used? And SFML doesn't use exceptions for errors. I prefer not to allow exceptional cases.

const_cast is a design issue regardless of the area it is used in.

You think that it is always a design issue, because you've never been in a situation where you need it. It's bad, but I need it for optimization purposes. Would you be happy if I replaced it with mutable members?

I highly doubt you need lazy evaluation for a View class. How many times do you expect the user to modify the View in a single frame? You'll be hitting performance issues much earlier with immediate-mode rendering than calling RecomputeMatrix() 2 or 3 extra times (1 call is ~14 additions and ~4 divisions, that's nothing).

Many things have changed in SFML 2, actually:
- immediate mode is gone
- there's no more GetRect() in sf::View
- I indeed use mutable members instead of const_cast in this class
Regarding performances, one can modify a view many times per frame, so I don't consider the lazy calculation of the view's matrix as useless.

The problem is: It is undefined behavior.

I'm not sure about the scope of this rule, but if it applies to any const variable then const_cast becomes pointless, doesn't it?

[Nexus]

I'm not sure about the scope of this rule, but if it applies to any const variable then const_cast becomes pointless, doesn't it?

Undefined behaviour emerges if the original variable (the object itsself, not any pointer or reference to it) is const-qualified, and this const is cast away. The reason for this rule is, once more in the C++ standard, implementation freedom: The compiler may treat constant objects in a special manner (for example regarding optimizations), while const_cast might injure the compiler's assumptions made for that purpose.

[Dravere]

The problem is: It is undefined behavior.

I'm not sure about the scope of this rule, but if it applies to any const variable then const_cast becomes pointless, doesn't it?

No it doesn't. Some examples:

Code: [Select]


struct Point
{
    float x;
    float y;
};

int main()
{
    const Point constantPoint = { 0.0f, 0.0f };
    const_cast<Point&>(constantPoint).x = 1.0f; // undefined behavior

    Point myPoint = { 0.0f, 0.0f };
    Point const& myPointNowConstant = myPoint;

    const_cast<Point&>(myPointNowConstant).x = 1.0f; // absoluty legal and defined.

    // If a bady interface is there or an old C one with no const,
    // the following is also correct:
    Point* thisIsATransport = const_cast<Point*>(&constantPoint);
    // ... there will be no modification through thisIsATransport
    Point const* pointerToConstantPoint = thisIsATransport;

    return 0;
}


// Or for exmpale this one is also legal and often used:
class MyClass
{
public:
    AnObject const& operator [](std::size_t index) const
    {
        // whatever ...
    }

    AnObject& operator [](std::size_t index)
    {
        return
            const_cast<AnObject&>(
                (*static_cast<MyClass const*>(this))[index]);
    }
};

Dravere

[nullsquared]

No, smart pointers.

That would indeed solve the problems I mentionned. But do we really need that extra level of complexity (it may not be complex for you and me, but it is for beginners)?

Sure, why not?  Beginners know how to program graphical applications, but don't know how to unzip boost somewhere and type #include <boost/shared_ptr.hpp>?

So use a pointer to a const View. Throw an exception on a NULL view.

Why would I allow something and thrown an exception when it is used? And SFML doesn't use exceptions for errors. I prefer not to allow exceptional cases.

Ok.

const_cast is a design issue regardless of the area it is used in.

You think that it is always a design issue, because you've never been in a situation where you need it. It's bad, but I need it for optimization purposes. Would you be happy if I replaced it with mutable members?

Put aside what I think.  Then insert what professionals think:
http://www.parashift.com/c++-faq-lite/const-correctness.html#faq-18.13
Notice:

If you ever want to use const_cast, use mutable instead. In other words, if you ever need to change a member of an object, and that object is pointed to by a pointer-to-const, the safest and simplest thing to do is add mutable to the member's declaration. You can use const_cast if you are sure that the actual object isn't const (e.g., if you are sure the object is declared something like this: Set s;), but if the object itself might be const (e.g., if it might be declared like: const Set s;), use mutable rather than const_cast.

This code:

Code: [Select]


const sf::View theView(sf::FloatRect(left, right, top, bottom));
theRT.SetView(theView);

Would result in undefined behaviour according to the standard.  (BTW, it's only undefined if the original object is const - const reference/const pointer to a non-const variable will not be undefined.)

By your design choices, "why let the user do this if they shouldn't?" Then the reference should be non-const Non-const reference will make the user think "hey, it takes a reference to my object, it's probably a bad idea to make it scoped locally."

Many things have changed in SFML 2, actually:
- immediate mode is gone
- there's no more GetRect() in sf::View
- I indeed use mutable members instead of const_cast in this class

Cool, thanks

I'm not sure about the scope of this rule, but if it applies to any const variable then const_cast becomes pointless, doesn't it?

Yes, it does become useless.  Thus, the mutable keyword.

And force heap allocation plus overhead for shared semantics? No, that's no real alternative.

Oh no, not HEAP ALLOCATION and SHARED OVERHEAD!  How in the world are we going to make it run on my calculator if we use THOSE?! (BTW, do you understand that std::vector uses heap allocation (and lots of it) internally?)

Don't forget RVO.

What's that?

If the design looked the way you liked it, there were other people not being content.

I'd love to hear the counter-arguments.

[Nexus]

Sure, why not?  Beginners know how to program graphical applications, but don't know how to unzip boost somewhere and type #include <boost/shared_ptr.hpp>?

Adding Boost dependency is another thing that's not compatible with SFML's philosophy.

And force heap allocation plus overhead for shared semantics? No, that's no real alternative.

Oh no, not HEAP ALLOCATION and SHARED OVERHEAD!  How in the world are we going to make it run on my calculator if we use THOSE?!

Don't make fun of my statement, I am indeed serious. With your approach, it is not possible anymore to initialize a automatically-allocated object with a circle, for example. In this respect, the user is forced to give up value semantics and use pointers (either smart or not). In contrast to one copy that probably doesn't even take place, the need to dereference remains troughout the object lifetime, decreasing performance continuously (although in a small way). If one needs to assign a circle to an automatic object, an inevitable copy is performed, so we need a total of two constructions instead. Great deal for that restriction of flexibility.

Anyway, rvalue-references are going to make this topic redundant.
 

Don't forget RVO.

What's that?

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

I'd love to hear the counter-arguments.

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