SFML community forums
General => General discussions => Topic started by: nullsquared on December 30, 2009, 09:35:12 pm
-
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:
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:
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...
-
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.
-
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:
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?!?
-
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.
-
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:
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).
-
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 :)
-
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. :D
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. :P
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.
-
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 :)
-
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
-
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.
-
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?
-
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.
-
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:
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
-
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:
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.
-
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.
-
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.
-
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 (http://en.wikipedia.org/wiki/Return_value_optimization).
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.
-
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 (http://en.wikipedia.org/wiki/Return_value_optimization).
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."
-
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?
-
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?
-
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."
-
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.
-
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.
-
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:
window.SetView(NULL);
is a much more visible error than this:
{
sf::View view(...);
window.SetView(view);
}
It's extremely hard to track down the second problem.
Or even worse:
window.SetView(sf::View(...));
-
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 :P
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:
-
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. :)
-
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:
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
-
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 :)
-
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)
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)
// 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 :)
-
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
-
I already thought about using pimpl.
It would help for:
- binary backward compatibility
- hiding all the ugly private details (which is the main purpose of the technique)
- allowing easy ref-counted sharing and COW if I need it (like for resources classes)
- removing the need for const_cast or mutable, indeed; but only with an implementation that doesn't forward constness to the private pointer
I recently implemented pimpl_ptr classes at work, and things can be encapsulated enough so that it is safe enough to use this idiom.
I'm still not sure whether I should use it or not, though.
-
You need to implement your own copy-constructor and operator.
Yes. I don't understand how that is an issue? Don't avoid features of the language for no reason.
but only with an implementation that doesn't forward constness to the private pointer
What do you mean? The pointer itself is const, as in that it cannot change. However, it is not a const pointer to a const instance, which means you can modify whatever it points to.
I recently implemented pimpl_ptr classes at work, and things can be encapsulated enough so that it is safe enough to use this idiom.
What's a pimpl_ptr? A shared_ptr works fine...?
-
Yes. I don't understand how that is an issue? Don't avoid features of the language for no reason.
It is not a feature of the language, it is a workaround to reduce dependencies and/or hide private details, which the language can't do by itself.
If you use it to avoid mutable or const_cast it is even more a hack than a solution. Don't forget that pimpl also adds several overhead. You need additional indirection, lose inlining and have additional heap allocation. Especially with small objects this can be a problem, because the heap often isn't optimized for small object but for big ones.
What do you mean? The pointer itself is const, as in that it cannot change. However, it is not a const pointer to a const instance, which means you can modify whatever it points to.
Exaclty and this is what he meant. You can change data even the object should be const. So you lose the security at compile time.
What's a pimpl_ptr? A shared_ptr works fine...?
A pimpl_ptr probably is a class, that automaticly creates a new instance when it is copied. So you don't need to implement your own copy-constructor or operator=.
Dravere
-
What do you mean? The pointer itself is const, as in that it cannot change. However, it is not a const pointer to a const instance, which means you can modify whatever it points to.
You can wrap your private pointer into a class that forwards constness, like this:
template <typename T>
class PimplPtr
{
public :
T& operator *();
const T& operator *() const;
T* operator ->();
const T* operator ->() const;
...
};
class Class
{
public :
void ConstFunc() const
{
myData->some_member = 5; // error, operator -> forwards constness
}
private :
PimplPtr<ClassImpl> myData;
};
Doing so is a good idea, as it makes the use of an extra indirection completely transparent, you don't loose constness on the private members.
Which leads to the same conclusion as Dravere: using pimpl to get rid of constness is purely a hack, a good implementation should forward it like above.
Don't forget that pimpl also adds several overhead. You need additional indirection, lose inlining and have additional heap allocation. Especially with small objects this can be a problem, because the heap often isn't optimized for small object but for big ones.
Yup, that's why I don't use it at the moment.
A pimpl_ptr probably is a class, that automaticly creates a new instance when it is copied. So you don't need to implement your own copy-constructor or operator=.
Exactly. Just like shared_ptr or scoped_ptr, we can create a pimpl_ptr class that gives the same semantics as a raw pointer but with correct copy and assignment.
-
It is not a feature of the language, it is a workaround to reduce dependencies and/or hide private details, which the language can't do by itself.
Constructors and operator=() aren't part of the language?
If you use it to avoid mutable or const_cast it is even more a hack than a solution.
No it's not. It's a different design.
Don't forget that pimpl also adds several overhead.
Come back with some profiler results and I'll consider your argument.
Exaclty and this is what he meant. You can change data even the object should be const. So you lose the security at compile time.
C++ const objects are bitwise const, not logical const. Security of what is lost? We're talking about mutable members.
-
shared_ptrs are excellent to use however, there are reasons why a lot of game devs including the pros avoid them. No they don't care about the double heap allocations that boost does. They care about that just typing #include<boost/shared_ptr.hpp> increases the compile time of a already 3 day long compiled application. That is not even the main reason. A lot of game dev's avoid shared pointers because it does not allow them to explicitly control the life time of their heap allocated objects.
When you are creating resources and destroying resources at 60fps for say particle effects, maybe arrows that fire from a bow or bullets flying from a gun shared_ptrs are death to the code base because the objects linger longer then the should.
There is a whole article about it in general. At the end they give some reflections on shared_ptr and games.
http://www.gamasutra.com/view/feature/4015/managing_data_relationships.php
On another note Laurent did you look at Intrusive Pointers?
http://www.boost.org/doc/libs/1_41_0/libs/smart_ptr/intrusive_ptr.html
-
They care about that just typing #include<boost/shared_ptr.hpp> increases the compile time of a already 3 day long compiled application.
That's true, however, shared_ptr doesn't make that much of a slowdown. I'd worry more about the preprocessor library.
A lot of game dev's avoid shared pointers because it does not allow them to explicitly control the life time of their heap allocated objects.
somePtr.reset(); // magic
When you are creating resources and destroying resources at 60fps for say particle effects, maybe arrows that fire from a bow or bullets flying from a gun shared_ptrs are death to the code base because the objects linger long then the should linger.
theContainer.clear(); // magic
There is a whole article about it in general. At the end they give some reflections on shared_ptr and games.
http://www.gamasutra.com/view/feature/4015/managing_data_relationships.php
The whole point of shared_ptr is for ... shared objects. Not just memory management in general.
-
You should know even reset on the shared_ptr and clear on the container are not guaranteed to free the memory. Because if the ref count is not 0 the memory will remain allocated when you call reset().
[edit]
The point being those calls are not 100% reliable. Like I said shared_ptr's are great to use but they defiantly should not be over used. They are not the end all be all.
-
You should know even reset on the shared_ptr and clear on the container are not guaranteed to free the memory. Because if the ref count is not 0 the memory will remain allocated when you call reset().
[edit]
The point being those calls are not 100% reliable. Like I said shared_ptr's are great to use but they defiantly should not be over used. They are not the end all be all.
The whole point of shared_ptr is for ... shared objects. Not just memory management in general.
If you don't want to share the memory, then don't. shared_ptr's don't just sneak in some references out of no where so you can't free the memory...
-
Constructors and operator=() aren't part of the language?
I didn't say anything against copy-constructor or operator=(), so I didn't understand your argument there. That's why I thought you meant pimpl.
I only said that there is an additional possibility of failures, that was my point and nothing more.
No it's not. It's a different design.
Ok, it is a hack design or a bad design :)
Come back with some profiler results and I'll consider your argument.
This response is simply ... I don't know the best word for it in english ... perhaps it is better that way :|
You can also think a bit about performance without using a profiler. It is a fact that the windows heap that is used through new in C++ is optimized for bigger and slow for small objects.
That is also why Andrei Alexandrescu implements a small object allocator in his book Modern C++ Design. There are also other books pointing out, that the heap in C++ is very slow for small objects and you should better consider copying them.
But well ... if you simply want to ignore facts ... go on ...
C++ const objects are bitwise const, not logical const. Security of what is lost? We're talking about mutable members.
No we aren't or not only. When Laurent said he thought about using pimpl we changed to the use of pimpl for the whole class. Because only that way, you can have all those things that he listed ;)
Dravere
-
You should know even reset on the shared_ptr and clear on the container are not guaranteed to free the memory. Because if the ref count is not 0 the memory will remain allocated when you call reset().
[edit]
The point being those calls are not 100% reliable. Like I said shared_ptr's are great to use but they defiantly should not be over used. They are not the end all be all.
The whole point of shared_ptr is for ... shared objects. Not just memory management in general.
If you don't want to share the memory, then don't. shared_ptr's don't just sneak in some references out of no where so you can't free the memory...
Correct if you don't share the memory. But if you don't share the memory you should not be using shared pointers to begin with. No point in having that extra overhead when you can just used a scoped pointer.
-
Come back with some profiler results and I'll consider your argument.
You can also think a bit about performance without using a profiler. It is a fact that the windows heap that is used through new in C++ is optimized for bigger and slow for small objects.
If you're so worried about your heap being slow, you shouldn't even be using the default heap allocator. Use nedmalloc. And, obviously, don't allocate primitive types on the heap, that's just stupid. The whole point was to keep the mutable members in an internal structure, not all the members. If all the members need to be mutable then I believe a major redesign is in order.
But if you don't share the memory you should not be using shared pointers to begin with.
How is that any different from my original statement?? Like I said, "The whole point of shared_ptr is for ... shared objects. Not just memory management in general."
-
To add some other personal experience I can say that this...
window.SetView(sf::View(...));
...was so far one of the biggest headaches I ever had with using SFML because it did cost me so much time to find out what I did wrong. I'm pretty relieved that others did stumble about it, too, and that Laurent sees the issue.
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.
Personally I'm for totally for assign-by-value in this case as I don't see any benefit in doing it differently. Whenever the user wants to change the view somewhere there will be at least one assignment - why not do it right where it finally matters (inside CWindow) and in a way with the fewest possible usage errors (including no out-of-scope issue)?
I'm not going to use pointers because of personal design choices, but we all agree that it's not the only solution anyway.
Personally I don't need smart pointers, too.
Just as a sidenote: I do use Boost in combination with SFML myself but I actually do see a good benefit in SFML having no dependencies at all. No matter how easy or how complex a dependency is, it is just yet another thing one has to care about and keep it up-to-date and fix errors when updates brought breaking changes. In addition for a beginner Boost can easily look like a big, scary monster. Well, it might the most rewarding monster ever for a C++ beginner to face, but it can be scary nevertheless.