From: Oliver Kullmann (O.Kullmann_at_[hidden])
Date: 2006-06-27 14:40:49
On Tue, Jun 27, 2006 at 02:37:13PM +0100, Reece Dunn wrote:
> Oliver Kullmann wrote:
> > > NOTE: Instead of disabling the warnings, you can remove the warning by
> > > updating the code. For example:
> > >
> > > void foo( int bar ){} // unreferenced parameter
> > > void foo( int bar ){ bar; } // no warning!
> > >
> >
> > this is FALSE:
>
> No - it is one way to prevent the bug.
>
bug ? there is no bug here.
I guess you meant "warning" --- but your "repair" is meaningless,
since it introduces a meaningless expression.
And, for good reasons, g++ warns about this(!) (at least versions 4.0.2 and above):
Test.cpp: In function ‘void foo(int)’:
Test.cpp:1: warning: statement has no effect
One should not "repair" code just because of the stupidity of one compiler.
> > void foo(int){}
>
> This doesn't work if you have:
>
> /** @brief something
> * @param bar Oh dear, this is missing!
> */
> void foo( int bar ){}
>
> Either not specifying bar or commenting it out will mean that Doxygen will not
> be able to identify the bar parameter.
>
again, this has nothing to do with the code, but is a weakness of Doxygen;
so one can write a request w.r.t. Doxygen, but should not harm the code.
>
> > > class foo
> > > {
> > > bar & m_bar;
> > > foo( bar & b ): m_bar( b ){}
> > >
> > > // disable the no default compiler assignment operator warning:
> > > inline foo & operator=( const foo & );
> > > };
> >
> > again this is a perversion: the C++ should follow the LANGUAGE, and the special
> > handling of the *special member function* is important part of C++.
>
> No. You miss the issue here. The problem is that m_bar is a *reference* and
> *you can't assign a reference*!
>
yes, that what I mean: in this case explicitely disabling the copy assignment
is what is *needed*, and in this case a compiler warning is good, and helps
us improving the code.
> > In the above example, first "inline" shouldn't be there, and then it's not
> > about warnings, but about semantics: either we want the implicitly declared
> > copy assignment operator, and then we *must not* add that line, or we don't
> > want it, and then we *must* add the line --- there is no choice.
>
> The inline *should* be there.
I meant really the keyword "inline", that is, I meant it should be
// disable the no default compiler assignment operator warning:
foo & operator=( const foo & );
since in-class definitions are automatically inline, so in-class definitions
should never use the keyword "inline".
> This is like deriving from boost::noncopyable which
> uses the above trick to prevent copying. In this case, I don't think it is possible to
> use noncopyable as this is about the default assignment operator in foo. By adding
> the line:
>
> inline foo & operator=( const foo & );
>
> I am telling the compiler that I am providing my own assignment operator, so
> the compiler doesn't automatically generate one. This is the classic way to
> implement non-copyable behaviour (as well as providing a similar copy constructor).
>
I know --- as I said, the "inline" is not needed (and thus misleading --- "for what is
this there?" will the maintenance programmer ask --- "some strange language issues???").
> In fact, this highlights a potential issue in the code. The standard says that you
> can't change a reference, so the assignment operator will copy values not what
> is being pointed to. This can lead to a bug if you assign one foo to another. In
> this case the warning has identified a potential problem and the above will identify
> any places where that problem is manifest.
>
yes, this warning is justified. And different from the above examples, here we really
improve the code, by stating explicitely that this class has no copy assignment operator.
Oliver