Correct constness
Vadim
class Settings
{
public:
...
int GetValue() const;
void SetValue();
...
};
class ConfigManager
{
public:
...
Settings* GetSettings() const;
private:
Settings* s_;
};
Now, ConfigManager::GetSettings should return pointer to non-const
Settings, so caller can modify the settings. The question is whether
SetSettings itself should be const member or not?
Given the code above, it doesn't really matter, because it doesn't make
much difference (at least to me). Observable, non-observable state
etc... I couldn't find one srtrong argument for going one way or
another. I'm also reluctant having two different methods, one const and
another non-const.
Can anybody can give me a good reason for using (or not using) const?
Thanks,
Vadim
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
Andrei Alexandrescu (See Website For Email)
> I'm puzzled with the following simple scenario:
>
> class Settings
> {
> public:
> ...
> int GetValue() const;
> void SetValue();
> ...
> };
>
> class ConfigManager
> {
> public:
> ...
> Settings* GetSettings() const;
>
> private:
> Settings* s_;
> };
>
> Now, ConfigManager::GetSettings should return pointer to non-const
> Settings, so caller can modify the settings. The question is whether
> SetSettings itself should be const member or not?
>
> Given the code above, it doesn't really matter, because it doesn't make
> much difference (at least to me). Observable, non-observable state
> etc... I couldn't find one srtrong argument for going one way or
> another. I'm also reluctant having two different methods, one const and
> another non-const.
>
> Can anybody can give me a good reason for using (or not using) const?
I try to use const on a member whenever I can. It's a strong indication
about the object's invariant, and it helps checking the internals of the
class in addition to its clients.
Andrei
Tomás
> Now, ConfigManager::GetSettings should return pointer to non-const
> Settings, so caller can modify the settings. The question is whether
> SetSettings itself should be const member or not?
I see it simply:
Does the member function alter any of the member objects? If so, make it
non-const (you actually don't have a choice). Otherwise, make it const.
Here's an example of code I'd write:
class Monkey {
char* p;
public:
Monkey( char* const arg_p ) : p(arg_p) {}
void SetValue( char const c ) const /* Yes, const! */
{
*p = c;
}
void ChangeObject ( char* const arg_p ) /* Non-const! */
{
p = arg_p;
}
};
-Tomás
Dave Harris
> Now, ConfigManager::GetSettings should return pointer to non-const
> Settings, so caller can modify the settings.
That's debatable - it depends on whether the Settings are "part of" the
ConfigManager. Usually member variables like s_ do represent a "part of"
relationship, especially if they are "owning" (eg the ConfigManager is
responsible for deleting the settings). You don't show enough code for me
to be sure. The word "Manager", when used as a class name, can be a bit
vague.
If X is const, then anything which is part of X should also be const. So
for const-correctness you need to think carefully about what relationships
your pointers are implementing.
> The question is whether SetSettings itself should be const member
> or not?
It should not be const. The choice of which Settings the ConfigManager
uses is surely part of the visible state of the ConfigManager, and a
member function which changes the visible state should not be const.
> Given the code above, it doesn't really matter, because it doesn't make
> much difference (at least to me).
Is that because you will never actually have a reference to a const
ConfigManager object?
You should find the code:
void ConfigManager::SetSettings( Settings *s ) const {
s_ = s;
}
will not compile, which is surely a clue.
-- Dave Harris, Nottingham, UK.
Ralf Fassel
| void SetValue( char ... c ) const /* Yes, const! */
[edit to clarify which 'const' we are talking about]
This is a design question. If the class is about the pointer, then
const is ok. But if the class is really about what the pointer points
to, the const is not adequate, even if the class is _formally_ not
changed by a call to SetValue. Dewhurst suggests in his C++-Gotchas
to couple 'const' to the logical state of the class, and I tend to
agree here...
R'
Manfred von Willich
the methods, and the rest should follow.
Firstly, does the class semantically contain (as opposed to refer to)
the Settings object, as for example the data content of a "string" or
"vector" type? From the naming and your description, this does not
appear to be the intent. Accordingly, the SetSettings would also be
const if it does not change the ConfigManager object itself (i.e. if
the compiler does not complain), as you are not semantically modifying
its content, only what it refers to.
Secondly, you have not made it clear what SetSettings will do, as this
will affect its constness. It seems unlikely that it is to copy a
Settings into the Settings object pointed to by s_, since the
appropriate way of doing this from your description of GetSettings is
that you would do that as
*setmng.GetSettings() = SomeSettings;
or
setmng.GetSettings()->SetValue();
and a SetSettings should be removed. If SetSettings is supposed to set
the member pointer s_, then it clearly cannot be const.
Incidentally, I use the convention where possible for a
parameter/return type that a pointer implies a transfer of ownership,
and a reference implies no transfer. Accordingly, I would have changed
your GetSettings to
Settings& GetSettings() const { return *s_; }
Joshua Lehrer
Tomás wrote:
> Here's an example of code I'd write:
>
> class Monkey {
>
> char* p;
>
> public:
>
> Monkey( char* const arg_p ) : p(arg_p) {}
>
> void SetValue( char const c ) const /* Yes, const! */
> {
> *p = c;
> }
>
I disagree. This precludes you from changing the data member 'p' to be
a const-correct class, like an std::string.
I like to make my member functions const if they do not change the
observable value of the class, and non-const if they do. In this case,
SetValue clearly changes the observable value of the class and should
not be const.
Do you really want this to compile?
const Monkey m(p);
m.SetValue('C');
joshua lehrer
http://www.lehrerfamily.com/
Gene Bushuyev
wrote in message news:44652BC...@erdani.org...
Good point. Another reason to separate accessors and mutators is for the
purposes of encapsulation and information hiding. Functions that return
non-const references/pointers to internal data are almost as bad as making that
data public. The loosest coupling is provided by accessor functions that return
by value, which should always be preferred unless prohibitively expensive. There
is also a question of exception safety. Often accessors can provide higher
exception guarantee than mutators.
--
Gene Bushuyev (www.gbresearch.com)
----------------------------------------------------------------
There is no greatness where there is no simplicity, goodness and truth. ~ Leo
Tolstoy
Vadim
about.
If I rewrite my example this way:
class Settings
{
...
int GetValue() const;
void SetValue(int v);
};
class ConfigManager
{
....
const Settings* GetSettings() const;
Settings* GetSettings();
};
then it makes more sense to me. Seems that it was impossible to
accomplish with one SetSettings function. I'm taking my question back.
Vadim
James Dennett
>> Now, ConfigManager::GetSettings should return pointer to non-const
>> Settings, so caller can modify the settings. The question is whether
>> SetSettings itself should be const member or not?
>
>
> I see it simply:
>
> Does the member function alter any of the member objects? If so, make it
> non-const (you actually don't have a choice). Otherwise, make it const.
>
> Here's an example of code I'd write:
>
> class Monkey {
>
> char* p;
>
> public:
>
> Monkey( char* const arg_p ) : p(arg_p) {}
>
> void SetValue( char const c ) const /* Yes, const! */
> {
> *p = c;
> }
>
> void ChangeObject ( char* const arg_p ) /* Non-const! */
> {
> p = arg_p;
> }
> };
That's too simplistic to give the best results. If
std::string followed these rules, the only non-const
operations might be those that potentially changed
its size, and this also exposes too much in the way
of implementation details. For "pimpl" classes, it
would make almost every operation const, whereas it
makes much more sense for the const-ness of operations
on the pimpl/handle to match the const-ness of the
operations on the body. Physical design ought not
to have unfortunate effects on interfaces.
The rule you state puts const on operations that can
change the object's state (which is bad), and it fails
to put const on operations that don't affect the
logical state of the object (such as those which
might update an internal cache).
Without knowing what 'p' points to in your "example"
above, it's hard to say for sure that it's an example
of bad design, but it certainly gives cause for concern,
and wouldn't pass any code review I've seen.
-- James
Tomás
> I see it simply:
>
> Does the member function alter any of the member objects? If so, make it
> non-const (you actually don't have a choice). Otherwise, make it const.
I take that back. I was writing a class today which had dynamic memory
allocation, and I made a certain member function non-const even though I
it didn't alter any member objects (it did however alter memory which I
had dynamically allocated for an object of the class).
Something akin to the following:
class SomeClass {
unsigned char *p;
/* Address of dynamically allocated memory */
void SetSecondByte(unsigned char const val)
{
p[1] = val;
wrybred
> I'm puzzled with the following simple scenario:
>
> class Settings
> {
> public:
> ...
> int GetValue() const;
> void SetValue();
> ...
> };
>
> class ConfigManager
> {
> public:
> ...
> Settings* GetSettings() const;
>
> private:
> Settings* s_;
> };
>
> Now, ConfigManager::GetSettings should return pointer to non-const
> Settings, so caller can modify the settings. The question is whether
> SetSettings itself should be const member or not?
>
> Given the code above, it doesn't really matter, because it doesn't make
> much difference (at least to me). Observable, non-observable state
> etc... I couldn't find one srtrong argument for going one way or
> another. I'm also reluctant having two different methods, one const and
> another non-const.
>
> Can anybody can give me a good reason for using (or not using) const?
Do you really want settings to be changed by anyone at any time, by
virtue of exposing GetSettings()?
Const-correctness: really, just do it. Don't think about it. You've
insulated yourself from the problems that plague modern systems, such
as security, concurrency, and need to support legacy clients dependent
on a format you later regret exposing.
So setting aside the question of const-correctness, there are two
deeper design decisions:
1. Do you want to hard-code configuration values in a structure, even
if it's semi-opaque?
2. Do you want to give piecewise access to setting configuration
values?
1. If your system is fairly static, it's reasonable to have hard-coded
settings. If things are likely to change, then a dynamic API, using
something like Boost properties or a container of boost::any will give
more flexibility and less rework when the need arises for a new value.
2. Piecewise mutation of configuration values is very dangerous. It
allows clients to set things as they see fit, mucking with assumptions
that others make. Better to think in terms of a group of special
clients, such as a command line reader, XML configuratiion reader,
network-based daemon, etc., that can modify things. Then provide an
interface suitable for them and unsuitable for the rest of the world
that might be tempted to hack their way for their own ends.
Tomás
> I see it simply:
>
> Does the member function alter any of the member objects? If so, make it
> non-const (you actually don't have a choice). Otherwise, make it const.
I take that back. I wrote code a class the other day that had a member
function which altered dynamically allocated memory, and I declared the
function non-const.
Vadim
>the Settings object, as for example the data content of a "string" or
>"vector" type? From the naming and your description, this does not
>appear to be the intent.
My bad, I have to be more specific. ConfigManager and Settings were
random examples that came to my mind. In reality ConfigManager have a
different name and it represents network connectivity configuration
(configuration itself is serialized/deserialized to/from XML file).
Among other things it allows to specify usage of a proxy server. Think
of a a DLL (ownership transfer is not appropriate in this case) which
looks like this:
/* header file */
class IProxy
{
public:
...
uint16 GetPort() const = 0;
void SetPort(uint16 port) = 0;
void AddRef() const = 0;
void Release() const = 0;
...
};
class INetConfig
{
public:
....
IProxy* GetProxy() const = 0;
void AddRef() const = 0;
void Release() const = 0;
....
};
__declspec(dllexport) INetConfig* GetNetConfig();
/* end of header */
The only exported function is GetNetConfig through which caller can get
a pointer to INetConfig and read/change proxy settings. There's no
separate SetProxy. Proxy's settings are changed through pointer to
IProxy (SetPort method, for example).
Does INetConfig semantically contain IProxy? Hard to tell. Yes and no,
IMO. If I knew for sure, it would be easier for me to decide whether
GetProxy should be const or not. I did it const, as shown in example
above, but wasn't totally comfortable with it. I feel that IProxy is
part of INetConfig semantically and by changing proxy settings
INetConfig is changing as well...
This was my original question - whether INetConfig::GetProxy should be
const or not? It will compile and work in both cases: const and
non-const. However, since right now I don't have a strong opinion on
this, I wanted to develop one so I can follow it in the future when
encounter similar situation again.
Vadim
Gene Bushuyev
news:1147554969.2...@d71g2000cwd.googlegroups.com...
> Thanks for the reply, but I don't think this is what I was asking
> about.
>
> If I rewrite my example this way:
>
> class Settings
> {
> ...
> int GetValue() const;
> void SetValue(int v);
> };
>
> class ConfigManager
> {
> ....
> const Settings* GetSettings() const;
either this or better yet:
Settings GetSettings() const; // throw() if possible
And why would your class use the pointers at all?
> Settings* GetSettings();
this is not an accessor! Not only the name is confusing, but it also blows up
encapsulation; the ConfigManager surrendered all rights on its own Settings
object. Think about something like:
void SetSettings(const Settings&);
This is a contrived example, so the answers are conditioned on that. In many
cases the presence of getters/setters is an indication of poor design decisions:
the class doesn't sufficiently own its members.
--
Gene Bushuyev (www.gbresearch.com)
----------------------------------------------------------------
There is no greatness where there is no simplicity, goodness and truth. ~ Leo
Tolstoy
Vadim
Because this is an abstarct interface exposed from a library. I cannot
pass objects back and forth between module boundaries, as well as throw
exceptions. Please see my reply to Manfred (
http://groups.google.com/group/comp.lang.c++.moderated/browse_frm/thread/3418ef4017daf22d/d59c6bae0e81cbf0?lnk=arm&hl=en#d59c6bae0e81cbf0
). I tried to explain it in more details and used actual class names,
rather than Settings and ConfigManager.
Manfred von Willich
> > This was my original question - whether INetConfig::GetProxy should be
> > const or not?
Still trying to read between the lines, it seems as though the returned
IProxy object is semantically separate from the INetConfig entity -
once you have the IProxy pointer, whatever public methods IProxy has
are intended to be available. The INetConfig object is presumably
intended to administer which IProxy is current etc., but will not deal
with the details of its use. If this fits your picture, only methods
that change the "whichness" of the IProxy should be non-const (and
since GetProxy does not affect this, it should be const). If there is
interaction with an IProxy (other than selecting it) via the
INetConfig, you probably have muddled semantics, in which case you
should rethink the design of INetConfig.
Another aside (probably not applicable to this example): Blind yourself
to the ugliness of two versions of a method - a const and non-const
version. They are generally associated with semantically clean
constness. I would have been nicer if we could have something like:
int constness_of_object & MyArray::operator[] (int);
in the place of
int const & MyArray::operator[] (int) const;
int & MyArray::operator[] (int);
Manfred