const correctness thread

CuteAlien · Post by **CuteAlien** » Tue Jul 04, 2006 1:14 pm

Yeah i know there might be more important stuff...
Still i hope a thread for that stuff might be useful, as making an own topic for each function where const could be used seems like overkill.
As it's sometimes not that clear if a const should be used or not, i'll just list some and if they are included in svn i'll add an *fixed* later on and if the team disagrees i'll add an *rejected* to my list.

So here's a few functions where i think a const should be used. I changed it for testing already in my code and they don't seem to cause any problems:

The get-functions in ITexture (and also in: C3D8Texture.h/.cpp, C3D9Texture.h/.cpp, CNullDriver.h, COpenGLTextur.h/.cpp, CSoftwareTexture.h/.cpp, CSoftwareTexture2.h/.cpp):
virtual const core::dimension2d<s32> getOriginalSize() const = 0;
virtual const core::dimension2d<s32> getSize() const = 0;
virtual E_DRIVER_TYPE getDriverType() const = 0;
virtual ECOLOR_FORMAT getColorFormat() const = 0;
virtual s32 getPitch() const = 0;

In IGUIElement:
core::rect<s32> getRelativePosition() const
core::rect<s32> getAbsolutePosition() const
virtual bool isVisible() const
virtual bool isEnabled() const
virtual const wchar_t* getText() const
virtual s32 getID() const
EGUI_ELEMENT_TYPE getType() const

In IGUIButton (and also in CGUIButton.h/.cpp):
virtual bool isPressed() const = 0;
virtual bool getUseAlphaChannel() const = 0;

In IGUICheckBox (and also in CGUICheckBox.h/.cpp):
virtual bool isChecked() const = 0;

hybrid · Post by **hybrid** » Tue Jul 04, 2006 9:11 pm

I think we should use as many const qualifiers for methods as possible. Also const references in parameters should be used in some more places, e.g. for SColor passing. The only question is for const return values which might impose some trouble sometimes.

vitek · Post by **vitek** » Wed Jul 05, 2006 1:59 am

It isn't useful to use reference params for most native types because the size of the reference [pointer] is actually larger than that type. i.e. it isn't useful to pass a u8, c8, u16, s16, u32, s32 or f32 by const reference. Since SColor is just a s32, it doesn't make sense to use references when passing them as parameters.

hybrid · Post by **hybrid** » Wed Jul 05, 2006 7:32 am

No, SColor is actually a struct. Thus, it can easily grow beyond the s32 size, and furthermore have some additional data fields already due to the c++ runtime overhead. And I wouldn't make any bets that a pointer alignment is not faster than int alignment on some architecture. You never know...
Moreover: pointer is only larger if you're using 64bit systems, otherwise it's the same!

vitek · Post by **vitek** » Wed Jul 05, 2006 9:21 am

Sure, video::SColor is a struct. It just so happens to be one that is 4 bytes in size. Since there is no base class nor virtual table there is no C++ runtime overhead. Even though there is a lot of discussion going on about a flexible color format, I'd be willing to bet that the old video::SColor is here to stay as a 32-bit signed integer wrapper.

As for betting on pointer alignment being faster than int alignment on some architectures... That would be a waste. Are you going to change the parameter types based on architecture? Probably not.

Even if an int and a pointer are the same size, passing the integer by value should give better performance. You don't need to dereference an integer to get it's value.

And yes, I should have said "larger than or equal to", but you understood exactly the point I was trying to make.

Travis

CuteAlien · Post by **CuteAlien** » Wed Jul 05, 2006 11:28 am

Another struct where a const reference could make sense is SEvent in the OnEvent functions. I don't think an event will be changed anywhere after it was created, but i didn't try it out yet.

zenaku · Post by **zenaku** » Sat Jul 08, 2006 6:11 pm

I'm glad to see that everyone realizes the importance of const correctness!

Not only does it provide a better definition of the API, proper const correctness can be a great optimization.

Code: Select all

//original
virtual const core::dimension2d<s32> getOriginalSize() = 0; 

//this will perform faster when optimizations are enabled in the compiler
virtual const core::dimension2d<s32> getOriginalSize() const = 0;

CuteAlien · Post by **CuteAlien** » Wed Jul 19, 2006 12:44 pm

In vector3d.h:
bool equals(const vector3d<T>& other)
should be
bool equals(const vector3d<T>& other) const

vitek · Post by **vitek** » Wed Jul 19, 2006 3:28 pm

This is another issue, but it should probably be...

Code: Select all

bool epsilon_equals(const vector3d<T>& other, T epsilon) const;

And there should be a global epsilon_equals that allows the user to supply an epsilon value instead of always using ROUNDING_ERROR. They could be added as new methods if you didn't want to break compatibility.

Travis

jolindien · Post by **jolindien** » Tue Aug 08, 2006 10:51 pm

Another const problem, giving errors if you use common const "char*" retrun functions as argument :

files :
IGUITabControl.h
CGUITabControl.h
CGUITabControl.cpp

Code: Select all

virtual IGUITab* addTab(wchar_t* caption, s32 id=-1) = 0;

as it should be :

Code: Select all

virtual IGUITab* addTab( CONST wchar_t* caption, s32 id=-1) = 0;