Const-correctness help. Topic is solved

[scriptdaemon]

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]

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)

[scriptdaemon]

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.

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?

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]

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.

[scriptdaemon]

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]

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.

[TrV]

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]

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]

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

[scriptdaemon]

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]

It's not a big deal anyway scriptdaemon!

[Virchanza]

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]

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; }
};

[Virchanza]

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]

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