digiKam › digikam-devel

Logic error

Classic

List

Threaded

21 messages Options

Bugzilla from andi.clemens@gmx.net

Logic error

Hi,

void TemplateSelector::setTemplate(const Template& t) has two return
statements in it, although the return type is void.

We should change this.

Andi
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

Gilles Caulier-4

Re: Logic error

RIGHT !

Why GCC don't cry about ?

Let's go to fix it Andy (:=)))

Gilles

2010/11/21 Andi Clemens <[hidden email]>:

> Hi,
>
> void TemplateSelector::setTemplate(const Template& t) has two return
> statements in it, although the return type is void.
>
> We should change this.
>
> Andi
> _______________________________________________
> Digikam-devel mailing list
> [hidden email]
> https://mail.kde.org/mailman/listinfo/digikam-devel
>

_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

Bugzilla from andi.clemens@gmx.net

Re: Logic error

Well Eclipse "cried" about the error :D

Andi Clemens
-----------------
www.digikam.org

On Sunday 21 November 2010 19:59:07 Gilles Caulier wrote:

> RIGHT !
>
> Why GCC don't cry about ?
>
> Let's go to fix it Andy (:=)))
>
> Gilles
>
> 2010/11/21 Andi Clemens <[hidden email]>:
> > Hi,
> >
> > void TemplateSelector::setTemplate(const Template& t) has two return
> > statements in it, although the return type is void.
> >
> > We should change this.
> >
> > Andi
> > _______________________________________________
> > Digikam-devel mailing list
> > [hidden email]
> > https://mail.kde.org/mailman/listinfo/digikam-devel
>
> _______________________________________________
> Digikam-devel mailing list
> [hidden email]
> https://mail.kde.org/mailman/listinfo/digikam-devel

_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

Gilles Caulier-4

Re: Logic error

Which compiler is used in background with Eclipse ?

Gilles

2010/11/21 Andi Clemens <[hidden email]>:

> Well Eclipse "cried" about the error :D
>
>
> Andi Clemens
> -----------------
> www.digikam.org
>
> On Sunday 21 November 2010 19:59:07 Gilles Caulier wrote:
>> RIGHT !
>>
>> Why GCC don't cry about ?
>>
>> Let's go to fix it Andy (:=)))
>>
>> Gilles
>>
>> 2010/11/21 Andi Clemens <[hidden email]>:
>> > Hi,
>> >
>> > void TemplateSelector::setTemplate(const Template& t) has two return
>> > statements in it, although the return type is void.
>> >
>> > We should change this.
>> >
>> > Andi
>> > _______________________________________________
>> > Digikam-devel mailing list
>> > [hidden email]
>> > https://mail.kde.org/mailman/listinfo/digikam-devel
>>
>> _______________________________________________
>> Digikam-devel mailing list
>> [hidden email]
>> https://mail.kde.org/mailman/listinfo/digikam-devel
> _______________________________________________
> Digikam-devel mailing list
> [hidden email]
> https://mail.kde.org/mailman/listinfo/digikam-devel
>

_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

Bugzilla from andi.clemens@gmx.net

Re: Logic error

GCC... but the compiler doesn't matter, Eclipse has some code analyzing tools
that run on every compile. I never looked at their results before, so I never
recognized it :D

Andi Clemens
-----------------
www.digikam.org

On Sunday 21 November 2010 20:08:08 Gilles Caulier wrote:

> Which compiler is used in background with Eclipse ?
>
> Gilles
>
> 2010/11/21 Andi Clemens <[hidden email]>:
> > Well Eclipse "cried" about the error :D
> >
> >
> > Andi Clemens
> > -----------------
> > www.digikam.org
> >
> > On Sunday 21 November 2010 19:59:07 Gilles Caulier wrote:
> >> RIGHT !
> >>
> >> Why GCC don't cry about ?
> >>
> >> Let's go to fix it Andy (:=)))
> >>
> >> Gilles
> >>
> >> 2010/11/21 Andi Clemens <[hidden email]>:
> >> > Hi,
> >> >
> >> > void TemplateSelector::setTemplate(const Template& t) has two return
> >> > statements in it, although the return type is void.
> >> >
> >> > We should change this.
> >> >
> >> > Andi
> >> > _______________________________________________
> >> > Digikam-devel mailing list
> >> > [hidden email]
> >> > https://mail.kde.org/mailman/listinfo/digikam-devel
> >>
> >> _______________________________________________
> >> Digikam-devel mailing list
> >> [hidden email]
> >> https://mail.kde.org/mailman/listinfo/digikam-devel
> >
> > _______________________________________________
> > Digikam-devel mailing list
> > [hidden email]
> > https://mail.kde.org/mailman/listinfo/digikam-devel
>
> _______________________________________________
> Digikam-devel mailing list
> [hidden email]
> https://mail.kde.org/mailman/listinfo/digikam-devel

_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

Bugzilla from andi.clemens@gmx.net

Re: Logic error

In reply to this post by Bugzilla from andi.clemens@gmx.net

The same issue with CImg:

//! Save an empty .cimg file with specified dimensions.
static void save_empty_cimg(const char *const filename,
const unsigned int dx, const unsigned int
dy=1,
const unsigned int dz=1, const unsigned int
dv=1) {
return CImgList<T>::save_empty_cimg(filename,1,dx,dy,dz,dv);
}

//! Save an empty .cimg file with specified dimensions.
static void save_empty_cimg(cimg_std::FILE *const file,
const unsigned int dx, const unsigned int
dy=1,
const unsigned int dz=1, const unsigned int
dv=1) {
return CImgList<T>::save_empty_cimg(file,1,dx,dy,dz,dv);
}

Andi Clemens
-----------------
www.digikam.org

On Sunday 21 November 2010 16:19:58 Andi Clemens wrote:

_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

Bugzilla from andi.clemens@gmx.net

Re: Logic error

In reply to this post by Bugzilla from andi.clemens@gmx.net

Also we have 448 warnings about classes with virtual methods, but no virtual
destructors.

I guess these are valid, I will change this either today or after the 1.6.0
release.

Andi Clemens
-----------------
www.digikam.org

On Sunday 21 November 2010 16:19:58 Andi Clemens wrote:

_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

Bugzilla from andi.clemens@gmx.net

Re: Logic error

I added a "virtual" keyword to every destructor now (besides the clases from
3rdparty), to make things easier.
It shouldn't hurt to have virtual destructors, even when no virtual method is
in the base class.

But could it have disadvantages? Slowdowns of some kind?

The advantage would be to get rid of possible memory leaks due to wrong
destructor callings.

The patch is in a local branch, I will do further tests, but I'll wait for the
digiKam 1.6.0 release anyways, so please comment here if you think the patch
would be wrong and we should only add "virtual" to those classes with virtual
methods.

Andi Clemens
-----------------
www.digikam.org

On Sunday 21 November 2010 20:42:21 Andi Clemens wrote:

> Also we have 448 warnings about classes with virtual methods, but no
> virtual destructors.
>
> I guess these are valid, I will change this either today or after the 1.6.0
> release.
>
> Andi Clemens
> -----------------
> www.digikam.org
>
> On Sunday 21 November 2010 16:19:58 Andi Clemens wrote:
> > Hi,
> >
> > void TemplateSelector::setTemplate(const Template& t) has two return
> > statements in it, although the return type is void.
> >
> > We should change this.
> >
> > Andi
> > _______________________________________________
> > Digikam-devel mailing list
> > [hidden email]
> > https://mail.kde.org/mailman/listinfo/digikam-devel
>
> _______________________________________________
> Digikam-devel mailing list
> [hidden email]
> https://mail.kde.org/mailman/listinfo/digikam-devel

_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

Johannes Wienke-3

Re: Logic error

Am 21.11.2010 22:27 schrieb Andi Clemens:

> I added a "virtual" keyword to every destructor now (besides the clases from
> 3rdparty), to make things easier.
> It shouldn't hurt to have virtual destructors, even when no virtual method is
> in the base class.
>
> But could it have disadvantages? Slowdowns of some kind?
>
> The advantage would be to get rid of possible memory leaks due to wrong
> destructor callings.
>
> The patch is in a local branch, I will do further tests, but I'll wait for the
> digiKam 1.6.0 release anyways, so please comment here if you think the patch
> would be wrong and we should only add "virtual" to those classes with virtual
> methods.

It would sometimes cost an extra vtable. Probably neglectable.

What are you using for code analysis in eclipse?

Regards,
Johannes

_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

signature.asc (270 bytes) Download Attachment

Bugzilla from andi.clemens@gmx.net

Re: Logic error

cppcheck and the built-in "Run C/C++ Code Analysis"

Andi Clemens
-----------------
www.digikam.org

On Sunday 21 November 2010 22:36:47 Johannes Wienke wrote:

> Am 21.11.2010 22:27 schrieb Andi Clemens:
> > I added a "virtual" keyword to every destructor now (besides the clases
> > from 3rdparty), to make things easier.
> > It shouldn't hurt to have virtual destructors, even when no virtual
> > method is in the base class.
> >
> > But could it have disadvantages? Slowdowns of some kind?
> >
> > The advantage would be to get rid of possible memory leaks due to wrong
> > destructor callings.
> >
> > The patch is in a local branch, I will do further tests, but I'll wait
> > for the digiKam 1.6.0 release anyways, so please comment here if you
> > think the patch would be wrong and we should only add "virtual" to those
> > classes with virtual methods.
>
> It would sometimes cost an extra vtable. Probably neglectable.
>
> What are you using for code analysis in eclipse?
>
> Regards,
> Johannes

_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

Bugzilla from andi.clemens@gmx.net

Re: Logic error

In reply to this post by Bugzilla from andi.clemens@gmx.net

Basically since we inherit from QWidget most of the time, a virtual destructor
should always be used, since QWidget provides virtual methods?
Like I said I converted every destructor to be virtual in my branch and I can
not see any slowdowns.

Andi Clemens
-----------------
www.digikam.org

On Sunday 21 November 2010 22:27:55 Andi Clemens wrote:

> I added a "virtual" keyword to every destructor now (besides the clases
> from 3rdparty), to make things easier.
> It shouldn't hurt to have virtual destructors, even when no virtual method
> is in the base class.
>
> But could it have disadvantages? Slowdowns of some kind?
>
> The advantage would be to get rid of possible memory leaks due to wrong
> destructor callings.
>
> The patch is in a local branch, I will do further tests, but I'll wait for
> the digiKam 1.6.0 release anyways, so please comment here if you think the
> patch would be wrong and we should only add "virtual" to those classes
> with virtual methods.
>
>
> Andi Clemens
> -----------------
> www.digikam.org
>
> On Sunday 21 November 2010 20:42:21 Andi Clemens wrote:
> > Also we have 448 warnings about classes with virtual methods, but no
> > virtual destructors.
> >
> > I guess these are valid, I will change this either today or after the
> > 1.6.0 release.
> >
> > Andi Clemens
> > -----------------
> > www.digikam.org
> >
> > On Sunday 21 November 2010 16:19:58 Andi Clemens wrote:
> > > Hi,
> > >
> > > void TemplateSelector::setTemplate(const Template& t) has two return
> > > statements in it, although the return type is void.
> > >
> > > We should change this.
> > >
> > > Andi
> > > _______________________________________________
> > > Digikam-devel mailing list
> > > [hidden email]
> > > https://mail.kde.org/mailman/listinfo/digikam-devel
> >
> > _______________________________________________
> > Digikam-devel mailing list
> > [hidden email]
> > https://mail.kde.org/mailman/listinfo/digikam-devel
>
> _______________________________________________
> Digikam-devel mailing list
> [hidden email]
> https://mail.kde.org/mailman/listinfo/digikam-devel

_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

Johannes Wienke-3

Re: Logic error

In reply to this post by Bugzilla from andi.clemens@gmx.net

Hey,

Am 21.11.2010 22:39 schrieb Andi Clemens:
> cppcheck and the built-in "Run C/C++ Code Analysis"

How do I enable the code analysis? I cannot find anything in my Eclipse
for this.

Johannes

_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

signature.asc (270 bytes) Download Attachment

Bugzilla from andi.clemens@gmx.net

Re: Logic error

Hmm don't know, it is just there... :-)
I have the following (Archlinux) packages:

local/eclipse 3.6.1-1
An IDE for Java and other languages
local/eclipse-cdt 7.0.1-1
C/C++ Plugin for Eclipse
local/eclipse-mylyn 3.4.0-1
A task-focused interface for Eclipse

It is in the settings (search for "code") as well as in the context menu of
the project explorer.

Andi Clemens
-----------------
www.digikam.org

On Sunday 21 November 2010 23:29:55 Johannes Wienke wrote:
> Hey,
>
> Am 21.11.2010 22:39 schrieb Andi Clemens:
> > cppcheck and the built-in "Run C/C++ Code Analysis"
>
> How do I enable the code analysis? I cannot find anything in my Eclipse
> for this.
>
> Johannes
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

Johannes Wienke-3

Re: Logic error

Am 21.11.2010 23:36 schrieb Andi Clemens:

> Hmm don't know, it is just there... :-)
> I have the following (Archlinux) packages:
>
> local/eclipse 3.6.1-1
> An IDE for Java and other languages
> local/eclipse-cdt 7.0.1-1
> C/C++ Plugin for Eclipse
> local/eclipse-mylyn 3.4.0-1
> A task-focused interface for Eclipse
>
> It is in the settings (search for "code") as well as in the context menu of
> the project explorer.

It's not there. ;) And I use a hand installed completely up-to-date
eclipse. There must be something more in the arch package...

Johannes

_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

signature.asc (270 bytes) Download Attachment

Bugzilla from andi.clemens@gmx.net

Re: Logic error

Hmm actually it is part of CDT.
It has been there for a long time...

Andi Clemens
-----------------
www.digikam.org

On Sunday 21 November 2010 23:39:19 Johannes Wienke wrote:

> Am 21.11.2010 23:36 schrieb Andi Clemens:
> > Hmm don't know, it is just there... :-)
> > I have the following (Archlinux) packages:
> >
> > local/eclipse 3.6.1-1
> >
> > An IDE for Java and other languages
> >
> > local/eclipse-cdt 7.0.1-1
> >
> > C/C++ Plugin for Eclipse
> >
> > local/eclipse-mylyn 3.4.0-1
> >
> > A task-focused interface for Eclipse
> >
> > It is in the settings (search for "code") as well as in the context menu
> > of the project explorer.
>
> It's not there. ;) And I use a hand installed completely up-to-date
> eclipse. There must be something more in the arch package...
>
> Johannes

_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

Bugzilla from andi.clemens@gmx.net

Re: Logic error

In reply to this post by Johannes Wienke-3

See http://wiki.eclipse.org/CDT/designs/StaticAnalysis

Andi Clemens
-----------------
www.digikam.org

On Sunday 21 November 2010 23:39:19 Johannes Wienke wrote:

_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

Marcel Wiesweg

Re: Logic error

In reply to this post by Bugzilla from andi.clemens@gmx.net

> Basically since we inherit from QWidget most of the time, a virtual
> destructor should always be used, since QWidget provides virtual methods?

Yes, any QObject-derived class will already have a virtual destructor.

The simple rule is "make your destructor virtual if your class has any virtual
functions":
http://www.parashift.com/c++-faq-lite/virtual-functions.html#faq-20.7

As far as I see GCC always gives a warning if you have virtual methods and not
virtual destructor in a base class

There is a small group of classes which will not need a virtual destructor: If
a class has a private QSharedDataPointer d-pointer, then it will usually not
be subclassed and not be created on the heap.
And fast "inline" classes, like DColor, shouldnt have this.

> Like I said I converted every destructor to be virtual in my branch and I
> can not see any slowdowns.

I would say there is the "slowdown" of the indirect call through the vtable,
which should be negligible in most cases. For memory cost, I think every
object will get an additional pointer.

The question is, which classes remain if you keep aside all QObject-derived
ones, which have it already, and all implictly/explicily-shared classes, which
dont need it?

Marcel
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

Bugzilla from andi.clemens@gmx.net

Re: Logic error

But don't you have to declare it "virtual" again in all derived classes, although the base class already defined it like this?

If not, I guess we can skip all QObject / QWidget related classes, when I'm home I will check which classes remain if these types are ommitted.

Andi

-------- Original-Nachricht --------
> Datum: Mon, 22 Nov 2010 09:59:39 +0100
> Von: Marcel Wiesweg <[hidden email]>
> An: "digiKam developers room - digiKam - Manage your photographs as a\\ professional with the power of open source" <[hidden email]>
> Betreff: [Digikam-devel] Re: Logic error

>
> > Basically since we inherit from QWidget most of the time, a virtual
> > destructor should always be used, since QWidget provides virtual
> methods?
>
> Yes, any QObject-derived class will already have a virtual destructor.
>
> The simple rule is "make your destructor virtual if your class has any
> virtual
> functions":
> http://www.parashift.com/c++-faq-lite/virtual-functions.html#faq-20.7
>
> As far as I see GCC always gives a warning if you have virtual methods and
> not
> virtual destructor in a base class
>
> There is a small group of classes which will not need a virtual
> destructor: If
> a class has a private QSharedDataPointer d-pointer, then it will usually
> not
> be subclassed and not be created on the heap.
> And fast "inline" classes, like DColor, shouldnt have this.
>
> > Like I said I converted every destructor to be virtual in my branch and
> I
> > can not see any slowdowns.
>
> I would say there is the "slowdown" of the indirect call through the
> vtable,
> which should be negligible in most cases. For memory cost, I think every
> object will get an additional pointer.
>
> The question is, which classes remain if you keep aside all
> QObject-derived
> ones, which have it already, and all implictly/explicily-shared classes,
> which
> dont need it?
>
> Marcel
> _______________________________________________
> Digikam-devel mailing list
> [hidden email]
> https://mail.kde.org/mailman/listinfo/digikam-devel

--
GRATIS! Movie-FLAT mit über 300 Videos.
Jetzt freischalten unter http://portal.gmx.net/de/go/maxdome
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

Marcel Wiesweg

Re: Logic error

> But don't you have to declare it "virtual" again in all derived classes,
> although the base class already defined it like this?
>

Yes, a method is always virtual if declared in the base class, in the derived
class the keyword is cosmetic. Same source as above:

"Note: in a derived class, if your base class has a virtual destructor, your
own destructor is automatically virtual. You might need an explicit destructor
for other reasons, but there's no need to redeclare a destructor simply to
make sure it is virtual. No matter whether you declare it with the virtual
keyword, declare it without the virtual keyword, or don't declare it at all,
it's still virtual."

Marcel
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

Bugzilla from andi.clemens@gmx.net

Re: Logic error

Ok, hope this is true for every compiler out there... :-)
I will take a look at the code analysis later this evening.

Andi

-------- Original-Nachricht --------
> Datum: Mon, 22 Nov 2010 18:48:19 +0100
> Von: Marcel Wiesweg <[hidden email]>
> An: "digiKam developers room - digiKam - Manage your photographs as a\\ professional with the power of open source" <[hidden email]>
> Betreff: [Digikam-devel] Re: Logic error

>
> > But don't you have to declare it "virtual" again in all derived classes,
> > although the base class already defined it like this?
> >
>
> Yes, a method is always virtual if declared in the base class, in the
> derived
> class the keyword is cosmetic. Same source as above:
>
> "Note: in a derived class, if your base class has a virtual destructor,
> your
> own destructor is automatically virtual. You might need an explicit
> destructor
> for other reasons, but there's no need to redeclare a destructor simply to
> make sure it is virtual. No matter whether you declare it with the virtual
> keyword, declare it without the virtual keyword, or don't declare it at
> all,
> it's still virtual."
>
> Marcel
> _______________________________________________
> Digikam-devel mailing list
> [hidden email]
> https://mail.kde.org/mailman/listinfo/digikam-devel

--
GMX DSL Doppel-Flat ab 19,99 €/mtl.! Jetzt auch mit
gratis Notebook-Flat! http://portal.gmx.net/de/go/dsl
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel