Suspicious code in kdegraphics-3.5.2

[Frans Englich]

On Saturday 22 April 2006 11:16, Clarence Dang wrote:
> On Saturday 22 April 2006 09:36, Christoph Bartoschek wrote:
[...]

> > - kolourpaint/tools/kptoolselection.cpp:1095
> > - kolourpaint/tools/kptoolselection.cpp:1217
> >
> > && true ?
>
> Left in for debugging - easy to change to "&& false".

I think better debugging practices exists. That code ends up in production 
code and it is confusing.

I don't see any significant gain. The only difference is that one have to 
change "false" instead of adding "&& false", the times one debug that 
particular path.

According to my standards, debugging code shouldn't be ordinary code, it 
should either not end up in the repository in the first place, or be macros 
disabled with NDEBUG.

[...]

> > If m_mainWindow is NULL as indicated by line 1637, then line 1646
> > crashes.
>
> "Will never happen".  I am tempted simply to remove the "m_mainWindow"
> checks entirely as "m_mainWindow" can never be NULL.

If it's clear that it never can be null, such as that m_mainWindow is set in 
the constructor and that it's deleted in the destructor, I suggest:

* Declare m_mainWindow to be a const pointer("QMainWindow *const 
m_mainWindow;") and initialize it with constructor initialization.
* Add Q_ASSERTs do document and check that really is the case, in critical 
paths. This should hardly be necessary if you declare the pointer const.


Cheers,

		Frans