Const-correctness help. Topic is solved

If you are using the main C++ distribution of wxWidgets, Feel free to ask any question related to wxWidgets development here. This means questions regarding to C++ and wxWidgets, not compile problems.
Post Reply
scriptdaemon
Experienced Solver
Experienced Solver
Posts: 93
Joined: Sun Mar 15, 2009 10:28 pm

Const-correctness help.

Post by scriptdaemon » Sun Jun 13, 2010 8:47 pm

I can never find a definitive answer wherever I look, I've read that everything that can be const should be const, only references to pointers (for returning things in functions), etc. In the implementation for this class, I have made const everything that can be const. Is there anything that is const here absolutely unnecessary, or even should *not* be const (and why)? (All of these are static, by the way.)

Code: Select all

// -----------------------------------------------------------------------------
// Name:        winapi.cpp
// Author:      Kenny "Scriptdaemon" Williams
// Created:     3/14/2009
// Description: Windows-specific functions (implementation)
// -----------------------------------------------------------------------------

// TODO: Use enum for desktop styles
// TODO: Should I use a singleton or keep static methods?

#ifndef WX_PRECOMP
#   include <wx/msw/wrapwin.h>
#endif

#include "winapi.h"
#include <wx/msw/registry.h>
#include <wx/msw/wrapshl.h>
#include <wx/filename.h>
#include <wx/stdpaths.h>

// -----------------------------------------------------------------------------
// Public methods
// -----------------------------------------------------------------------------

const wxString WinApi::GetDesktopBackground()
{
    char buff[MAX_PATH];
    SystemParametersInfo(SPI_GETDESKWALLPAPER, MAX_PATH, &buff[0], 0);
    return wxString(buff, wxConvUTF8);
}

const wxString WinApi::GetUserPicsDir()
{
    LPITEMIDLIST pidl;
    const HRESULT hr = SHGetSpecialFolderLocation(NULL, CSIDL_MYPICTURES, &pidl);

    // This will also free pidl
    return SUCCEEDED(hr) ? wxItemIdList(pidl).GetPath() : *wxEmptyString;
}

void WinApi::SetDesktopBackground(const wxBitmap bmp, const wxString style)
{
    // If the bitmap object exists, save to user's pictures directory
    const wxString path = bmp.IsOk() ? SaveAsBmp(bmp) : GetDesktopBackground();

    // Set the style before updating the background
    SetDesktopBackgroundStyle(style);

    // Clear the current background first (releases lock on current bitmap)
    SystemParametersInfo(SPI_SETDESKWALLPAPER, 0, (PVOID)wxEmptyString, SPIF_UPDATEINIFILE | SPIF_SENDCHANGE);
    SystemParametersInfo(SPI_SETDESKWALLPAPER, 0, (PVOID)path.c_str(),  SPIF_UPDATEINIFILE | SPIF_SENDCHANGE);
}

void WinApi::SetRunKey(const bool addVal)
{
    wxRegKey key(wxRegKey::HKCU, "Software\\Microsoft\\Windows\\CurrentVersion\\Run");
    if (addVal)
    {
        key.SetValue(wxTheApp->GetAppName(), wxStandardPaths::Get().GetExecutablePath());
    }
    else
    {
        key.DeleteValue(wxTheApp->GetAppName());
    }
}

// -----------------------------------------------------------------------------
// Private methods
// -----------------------------------------------------------------------------

const wxString WinApi::SaveAsBmp(wxBitmap bmp)
{
    const wxFileName file(GetUserPicsDir(), wxTheApp->GetAppName(), "bmp");
    const wxString path = file.GetFullPath();
    bmp.SaveFile(path, wxBITMAP_TYPE_BMP);
    return path;
}

void WinApi::SetDesktopBackgroundStyle(const wxString style)
{
    wxRegKey key(wxRegKey::HKCU, "Control Panel\\Desktop");
    switch (style[0])
    {
        case 'S':  // Stretched
            key.SetValue("WallpaperStyle", "2");
            key.SetValue("TileWallpaper",  "0");
            break;

        case 'C':  // Centered
            key.SetValue("WallpaperStyle", "1");
            key.SetValue("TileWallpaper",  "0");
            break;

        case 'T':  // Tiled
            key.SetValue("WallpaperStyle", "1");
            key.SetValue("TileWallpaper",  "1");
            break;

        default: ; // Use previous style
    }
}

Auria
Site Admin
Site Admin
Posts: 6695
Joined: Thu Sep 28, 2006 12:23 am
Contact:

Post by Auria » Sun Jun 13, 2010 9:10 pm

Check method constness :

Code: Select all

const wxString WinApi::GetDesktopBackground() 
could probably be

Code: Select all

const wxString WinApi::GetDesktopBackground() const
As for const wxStrings, I suspect this is not very important, since wx strings are copy on write (i.e. their internal data is const). But const there does not hurt either.

Also,

Code: Select all

void WinApi::SetDesktopBackground(const wxBitmap bmp, const wxString style)
could be rewritten

Code: Select all

void WinApi::SetDesktopBackground(const wxBitmap& bmp, const wxString style)
since you're not modifying the bitmap, it's unecesary to make a copy of it, you can work directly on the original (hence the reference)
"Keyboard not detected. Press F1 to continue"
-- Windows

scriptdaemon
Experienced Solver
Experienced Solver
Posts: 93
Joined: Sun Mar 15, 2009 10:28 pm

Post by scriptdaemon » Sun Jun 13, 2010 9:28 pm

Auria wrote:Check method constness :

Code: Select all

const wxString WinApi::GetDesktopBackground() 
could probably be

Code: Select all

const wxString WinApi::GetDesktopBackground() const
I mentioned they're all static in the last parenthesis, so there's no object anyway.
Auria wrote:As for const wxStrings, I suspect this is not very important, since wx strings are copy on write (i.e. their internal data is const). But const there does not hurt either.
While it may not hurt, is there any difference at all keeping them there or removing them from all wxStrings?
Auria wrote: Also,

Code: Select all

void WinApi::SetDesktopBackground(const wxBitmap bmp, const wxString style)
could be rewritten

Code: Select all

void WinApi::SetDesktopBackground(const wxBitmap& bmp, const wxString style)
since you're not modifying the bitmap, it's unecesary to make a copy of it, you can work directly on the original (hence the reference)
I didn't think of that, thanks.

Auria
Site Admin
Site Admin
Posts: 6695
Joined: Thu Sep 28, 2006 12:23 am
Contact:

Post by Auria » Sun Jun 13, 2010 10:40 pm

scriptdaemon wrote:
Auria wrote:As for const wxStrings, I suspect this is not very important, since wx strings are copy on write (i.e. their internal data is const). But const there does not hurt either.
While it may not hurt, is there any difference at all keeping them there or removing them from all wxStrings?
I'm not a compiler writer, so can't tell for sure :) but I would suspect that keeping them const might yield a very small performance improvement by making some copies unecessary. but anyway when you copy a wxString around its contents are not copied, since it's COW. So that's very small.

For return types (e.g. const wxString Foo::getString()), I think this is useless clutter; since your method is returning an object and not a pointer, it should not matter that the caller of the method can write to the copy of the string.
"Keyboard not detected. Press F1 to continue"
-- Windows

scriptdaemon
Experienced Solver
Experienced Solver
Posts: 93
Joined: Sun Mar 15, 2009 10:28 pm

Post by scriptdaemon » Sun Jun 13, 2010 11:53 pm

Auria wrote:For return types (e.g. const wxString Foo::getString()), I think this is useless clutter; since your method is returning an object and not a pointer, it should not matter that the caller of the method can write to the copy of the string.
Ah, so go ahead and remove the const from before each function that has that? Is that a general rule then to Ooly put const in the beginning of a function declaration if it returns a pointer? (Are there no performance gains the way I have it now, even if slightly?)

TrV
Ultimate wxWidgets Guru
Ultimate wxWidgets Guru
Posts: 630
Joined: Wed Jul 04, 2007 1:12 pm

Post by TrV » Sun Jun 13, 2010 11:57 pm

I do agree with Auria.

As a "protocol", here's what i follow when coding:
- If a copy is made (neither "&" nor "*") "const" is out of the point because everything is local, so one guesses that developer knows what he wants and what he needs
- If no copy is made and local work is done with original variable ("&" or "*"):
. value must be changed? no "const" of course
. value should not be changed? make it read-only with const
- For a method, if no W access is performed on attributes, make the method "const": type method_name(arg1, arg2,) const.
Last edited by TrV on Mon Jun 14, 2010 12:03 am, edited 1 time in total.

TrV
Ultimate wxWidgets Guru
Ultimate wxWidgets Guru
Posts: 630
Joined: Wed Jul 04, 2007 1:12 pm

Post by TrV » Mon Jun 14, 2010 12:02 am

scriptdaemon wrote:Is that a general rule then to Ooly put const in the beginning of a function declaration if it returns a pointer? (Are there no performance gains the way I have it now, even if slightly?)
Returning a pointer is a special case that has nothing to do with gaining performance. Doing that, memory leak risks are high and developer must be very careful to keep returned address until he frees its memory.
Knowing that, you generally return a copy, so "const" is useless.

scriptdaemon
Experienced Solver
Experienced Solver
Posts: 93
Joined: Sun Mar 15, 2009 10:28 pm

Post by scriptdaemon » Mon Jun 14, 2010 12:11 am

TrV wrote:I do agree with Auria.

As a "protocol", here's what i follow when coding:
- If a copy is made (neither "&" nor "*") "const" is out of the point because everything is local, so one guesses that developer knows what he wants and what he needs
- If no copy is made and local work is done with original variable ("&" or "*"):
. value must be changed? no "const" of course
. value should not be changed? make it read-only with const
- For a method, if no W access is performed on attributes, make the method "const": type method_name(arg1, arg2,) const.
The first one was the only one I didn't fully understand until you said it. Thanks guys.

EDIT: Oops. I wanted to mark yours as assisting, but it wouldn't let me do that after marking Auria's first post as accepted. I suppose I have to mark the assisting first then?

Auria
Site Admin
Site Admin
Posts: 6695
Joined: Thu Sep 28, 2006 12:23 am
Contact:

Post by Auria » Mon Jun 14, 2010 12:30 am

scriptdaemon wrote:
EDIT: Oops. I wanted to mark yours as assisting, but it wouldn't let me do that after marking Auria's first post as accepted. I suppose I have to mark the assisting first then?
I fixed it for you :) but yeah, the current system does not allow posts to be marked as assisting after one is accepted
"Keyboard not detected. Press F1 to continue"
-- Windows

scriptdaemon
Experienced Solver
Experienced Solver
Posts: 93
Joined: Sun Mar 15, 2009 10:28 pm

Post by scriptdaemon » Mon Jun 14, 2010 12:40 am

Auria wrote:
scriptdaemon wrote:
EDIT: Oops. I wanted to mark yours as assisting, but it wouldn't let me do that after marking Auria's first post as accepted. I suppose I have to mark the assisting first then?
I fixed it for you :) but yeah, the current system does not allow posts to be marked as assisting after one is accepted
Thanks. :)

That should probably be adjusted, perhaps in a case where a user adds extra insight to a thread after a solution has already been accepted (or, in my case, what just happened. =P).

TrV
Ultimate wxWidgets Guru
Ultimate wxWidgets Guru
Posts: 630
Joined: Wed Jul 04, 2007 1:12 pm

Post by TrV » Mon Jun 14, 2010 6:39 am

It's not a big deal anyway scriptdaemon! ;)

Virchanza
Experienced Solver
Experienced Solver
Posts: 78
Joined: Sun Jul 19, 2009 6:12 am

Post by Virchanza » Mon Jun 14, 2010 10:09 am

I never mark the return value of a function as const. There's no point in doing so unless you have a specific reason which depends on the inner workings of the class.

I use const where-ever and whenever possible, except for these two cases:
1) The return values of functions
2) When casting to another type ( e.g. Func( (int)i ); )

The return value of a function is an R-value, which pretty much means that it cannot appear on the left-hand-side of an assignment statement (the compiler also prohibits you from taking the address of an R-value). Here's an example of code that won't compile:

Code: Select all

int Func()
{
    return 5;
}

int main()
{
    Func() = 7;
}
The above code won't compile because the return value from Func is an R-value. It's got nothing to do with constness.

Here's a good example showing how R-values aren't const:

Code: Select all

class MyClass {
protected:
    int member;

public:

    MyClass &operator=(int const arg) /* Note that this method is non-const */
    {
        member = arg;
    }
};

MyClass SomeFunc1()
{
    MyClass dummy;

    return dummy;
}

int SomeFunc2()
{
    return 5;
}

int main()
{

    /* The following two will compile fine */
    SomeFunc1() = 5;
    SomeFunc1().operator=(5);

    /* The following won't compile */

    SomeFunc2() = 4;
}
The last line won't compile because you have an R-value appearing on the left-hand-side of an assignment statement.

You might be surprised that the first one compiles, i.e.

SomeFunc1() = 5;

This compiles because it is automagically transformed into:

SomeFunc1().operator=(5);

...and now it's a function call instead of an assignment!

You're right to say that you should use const whenever you can... but my own recommendation is to not bother using const for R-values as it just clutters the code.

Auria
Site Admin
Site Admin
Posts: 6695
Joined: Thu Sep 28, 2006 12:23 am
Contact:

Post by Auria » Mon Jun 14, 2010 11:55 pm

Virchanza: constness after return values are useful when you return pointers

Code: Select all


class X
{
private:
    Y* m_my_precious_data;
public:

    /** I let you set it but not change it! */
    const Y* getPreciousData() const { return m_my_precious_data; }
};

"Keyboard not detected. Press F1 to continue"
-- Windows

Virchanza
Experienced Solver
Experienced Solver
Posts: 78
Joined: Sun Jul 19, 2009 6:12 am

Post by Virchanza » Tue Jun 15, 2010 9:51 am

Auria wrote:Virchanza: constness after return values are useful when you return pointers

Code: Select all


class X
{
private:
    Y* m_my_precious_data;
public:

    /** I let you set it but not change it! */
    const Y* getPreciousData() const { return m_my_precious_data; }
};

Ah yes I agree about putting "const" on the "pointed-to" type, but not about putting it on the pointer itself.

For example:

Code: Select all

int some_global_variable;

int const *Func1()
{
    /* This make sense if you don't want
       the caller to edit the pointed-to
       data */

    return &some_global_variable;
}

int *const Func2()
{
    /* This is redundant because the pointer
       returned from this function is an
       R-value anyway */

    return &some_global_variable;
}

orbitcowboy
I live to help wx-kind
I live to help wx-kind
Posts: 178
Joined: Mon Jul 23, 2007 9:01 am

Post by orbitcowboy » Tue Jun 15, 2010 3:05 pm

You ever tried cppcheck?

Its an open source static code analysis tool that identifies const-correctness issues and many others. You can download it form:

http://sourceforge.net/projects/cppcheck/

Best regards

Orbitcowboy
OS: Ubuntu 9.04 (32/64-Bit), Debian Lenny (32-Bit)
Compiler: gcc/g++-4.3.3 , gcc/g++-4.4.0
wxWidgets: 2.8.10,2.9.0

Post Reply