MetaEngine::save() is re-implemented in DMetadata without to use virtual property.

[Gilles Caulier-4]

Hi, all,

I currently review all Clazy static analyzer reports, and i plan to
merge DMetadata class to MetaEngine.

While reviewing the code I discovered that MetadaEngine::save() is
implemented into DMetadata without being declared as virtual.

I'm surprised to not see a G++ warning about this code, which can
introduce side effects... Look by yourself :

https://invent.kde.org/graphics/digikam/-/blob/master/core/libs/metadataengine/engine/metaengine.h#L372

https://invent.kde.org/graphics/digikam/-/blob/master/core/libs/metadataengine/dmetadata/dmetadata.h#L95

How is it technically possible ?

Best

Gilles Caulier

[Gilles Caulier-4]

And it's the same for MetaEngine::applyChanges() ....

Gilles

Le ven. 13 nov. 2020 à 16:10, Gilles Caulier
<[hidden email]> a écrit :

>
> Hi, all,
>
> I currently review all Clazy static analyzer reports, and i plan to
> merge DMetadata class to MetaEngine.
>
> While reviewing the code I discovered that MetadaEngine::save() is
> implemented into DMetadata without being declared as virtual.
>
> I'm surprised to not see a G++ warning about this code, which can
> introduce side effects... Look by yourself :
>
> https://invent.kde.org/graphics/digikam/-/blob/master/core/libs/metadataengine/engine/metaengine.h#L372
>
> https://invent.kde.org/graphics/digikam/-/blob/master/core/libs/metadataengine/dmetadata/dmetadata.h#L95
>
> How is it technically possible ?
>
> Best
>
> Gilles Caulier

[Gilles Caulier-4]

A partial response can be found here :

https://stackoverflow.com/questions/1853896/is-it-possible-to-override-a-non-virtual-method

This is typically possible but highly not recommended and not portable
everywhere... as i understand...

Gilles

Le ven. 13 nov. 2020 à 16:11, Gilles Caulier

<[hidden email]> a écrit :

>
> And it's the same for MetaEngine::applyChanges() ....
>
> Gilles
>
> Le ven. 13 nov. 2020 à 16:10, Gilles Caulier
> <[hidden email]> a écrit :
> >
> > Hi, all,
> >
> > I currently review all Clazy static analyzer reports, and i plan to
> > merge DMetadata class to MetaEngine.
> >
> > While reviewing the code I discovered that MetadaEngine::save() is
> > implemented into DMetadata without being declared as virtual.
> >
> > I'm surprised to not see a G++ warning about this code, which can
> > introduce side effects... Look by yourself :
> >
> > https://invent.kde.org/graphics/digikam/-/blob/master/core/libs/metadataengine/engine/metaengine.h#L372
> >
> > https://invent.kde.org/graphics/digikam/-/blob/master/core/libs/metadataengine/dmetadata/dmetadata.h#L95
> >
> > How is it technically possible ?
> >
> > Best
> >
> > Gilles Caulier

[Marcel Mathis]

Hi Gilles

This is completely valid code if the virtual is missing. So I don't think there is a compiler error or warning. See the attached example.

I would advise to use the compiler flag -Werror=suggest-override. So you can guarantee that all overridden functions are at least virtual.

Hope to help you with this information.

Cheers

Marcel

On Fri, Nov 13, 2020 at 4:16 PM Gilles Caulier <[hidden email]> wrote:

A partial response can be found here :

https://stackoverflow.com/questions/1853896/is-it-possible-to-override-a-non-virtual-method

This is typically possible but highly not recommended and not portable
everywhere... as i understand...

Gilles

Le ven. 13 nov. 2020 à 16:11, Gilles Caulier

<[hidden email]> a écrit :
>
> And it's the same for MetaEngine::applyChanges() ....
>
> Gilles
>
> Le ven. 13 nov. 2020 à 16:10, Gilles Caulier
> <[hidden email]> a écrit :
> >
> > Hi, all,
> >
> > I currently review all Clazy static analyzer reports, and i plan to
> > merge DMetadata class to MetaEngine.
> >
> > While reviewing the code I discovered that MetadaEngine::save() is
> > implemented into DMetadata without being declared as virtual.
> >
> > I'm surprised to not see a G++ warning about this code, which can
> > introduce side effects... Look by yourself :
> >
> > https://invent.kde.org/graphics/digikam/-/blob/master/core/libs/metadataengine/engine/metaengine.h#L372
> >
> > https://invent.kde.org/graphics/digikam/-/blob/master/core/libs/metadataengine/dmetadata/dmetadata.h#L95
> >
> > How is it technically possible ?
> >
> > Best
> >
> > Gilles Caulier

[Gilles Caulier-4]

Hi Marcel,

Yes the code is valid, but so far wrong in the concept of oriented
objects programming.

I know the suggested override compiler method, and it's activated here.

Also all static analyzers must report this kind of overload method,
but nothing appears...

Best

Gilles

Le ven. 13 nov. 2020 à 17:41, Marcel Mathis <[hidden email]> a écrit :

>
> Hi Gilles
>
> This is completely valid code if the virtual is missing. So I don't think there is a compiler error or warning. See the attached example.
>
> I would advise to use the compiler flag -Werror=suggest-override. So you can guarantee that all overridden functions are at least virtual.
>
> Hope to help you with this information.
>
> Cheers
> Marcel
>
> On Fri, Nov 13, 2020 at 4:16 PM Gilles Caulier <[hidden email]> wrote:
>>
>> A partial response can be found here :
>>
>> https://stackoverflow.com/questions/1853896/is-it-possible-to-override-a-non-virtual-method
>>
>> This is typically possible but highly not recommended and not portable
>> everywhere... as i understand...
>>
>> Gilles
>>
>> Le ven. 13 nov. 2020 à 16:11, Gilles Caulier
>>
>> <[hidden email]> a écrit :
>> >
>> > And it's the same for MetaEngine::applyChanges() ....
>> >
>> > Gilles
>> >
>> > Le ven. 13 nov. 2020 à 16:10, Gilles Caulier
>> > <[hidden email]> a écrit :
>> > >
>> > > Hi, all,
>> > >
>> > > I currently review all Clazy static analyzer reports, and i plan to
>> > > merge DMetadata class to MetaEngine.
>> > >
>> > > While reviewing the code I discovered that MetadaEngine::save() is
>> > > implemented into DMetadata without being declared as virtual.
>> > >
>> > > I'm surprised to not see a G++ warning about this code, which can
>> > > introduce side effects... Look by yourself :
>> > >
>> > > https://invent.kde.org/graphics/digikam/-/blob/master/core/libs/metadataengine/engine/metaengine.h#L372
>> > >
>> > > https://invent.kde.org/graphics/digikam/-/blob/master/core/libs/metadataengine/dmetadata/dmetadata.h#L95
>> > >
>> > > How is it technically possible ?
>> > >
>> > > Best
>> > >
>> > > Gilles Caulier

[Maik Qualmann]

I think the code is completely correct. If we make save() virtual from
MetaEngine, a save() call within the base class of MetaEngine would first call
save() from DMetadata. That doesn't always have to be desired.

Maik

Am Freitag, 13. November 2020, 18:01:11 CET schrieb Gilles Caulier:

> Hi Marcel,
>
> Yes the code is valid, but so far wrong in the concept of oriented
> objects programming.
>
> I know the suggested override compiler method, and it's activated here.
>
> Also all static analyzers must report this kind of overload method,
> but nothing appears...
>
> Best
>
> Gilles
>
> Le ven. 13 nov. 2020 à 17:41, Marcel Mathis <[hidden email]> a écrit :
> > Hi Gilles
> >
> > This is completely valid code if the virtual is missing. So I don't think
> > there is a compiler error or warning. See the attached example.
> >
> > I would advise to use the compiler flag -Werror=suggest-override. So you
> > can guarantee that all overridden functions are at least virtual.
> >
> > Hope to help you with this information.
> >
> > Cheers
> > Marcel
> >
> > On Fri, Nov 13, 2020 at 4:16 PM Gilles Caulier <[hidden email]>

wrote:

> >> A partial response can be found here :
> >>
> >> https://stackoverflow.com/questions/1853896/is-it-possible-to-override-a-> >> non-virtual-method
> >>
> >> This is typically possible but highly not recommended and not portable
> >> everywhere... as i understand...
> >>
> >> Gilles
> >>
> >> Le ven. 13 nov. 2020 à 16:11, Gilles Caulier
> >>
> >> <[hidden email]> a écrit :
> >> > And it's the same for MetaEngine::applyChanges() ....
> >> >
> >> > Gilles
> >> >
> >> > Le ven. 13 nov. 2020 à 16:10, Gilles Caulier
> >> >
> >> > <[hidden email]> a écrit :
> >> > > Hi, all,
> >> > >
> >> > > I currently review all Clazy static analyzer reports, and i plan to
> >> > > merge DMetadata class to MetaEngine.
> >> > >
> >> > > While reviewing the code I discovered that MetadaEngine::save() is
> >> > > implemented into DMetadata without being declared as virtual.
> >> > >
> >> > > I'm surprised to not see a G++ warning about this code, which can
> >> > > introduce side effects... Look by yourself :
> >> > >
> >> > > https://invent.kde.org/graphics/digikam/-/blob/master/core/libs/metad
> >> > > ataengine/engine/metaengine.h#L372
> >> > >
> >> > > https://invent.kde.org/graphics/digikam/-/blob/master/core/libs/metad
> >> > > ataengine/dmetadata/dmetadata.h#L95
> >> > >
> >> > > How is it technically possible ?
> >> > >
> >> > > Best
> >> > >
> >> > > Gilles Caulier

[Rob D]

Digikam has started to crash when tagging faces.  This started after building from sources pulled after Maik's file download fixes, but lots of other changes were pushed around the same time.

When a face is recognized, if you apply the face tag to the image, DK immediately crashes.

Here is the log output: (TAGs have been XXX'd for privacy)

qt.text.layout: do layout

qt.accessibility.cache: insert - id: 2147485717 iface: QAccessibleInterface(0x14ab10e0 name="Applying face changes" role=StaticText obj=QLabel(0x17f82bf0)"invisible")

digikam.general: Found FacesEngine identity 2568 for tag 4310

qt.accessibility.cache: insert - id: 2147485718 iface: QAccessibleInterface(0x1046d980 name="Assigning image tags" role=StaticText obj=QLabel(0x17f381e0)"invisible")

digikam.general: Scheduled to write

digikam.general: No write to baloo +++++++++++++++++++++++++++++++++++++

qt.accessibility.cache: insert - id: 2147485719 iface: QAccessibleInterface(0x103fe940 name="Writing metadata to files" role=StaticText obj=QLabel(0x12a2c770)"invisible")

digikam.metaengine: Set face region: 0.435847 0.360615 0.301257 0.230655

digikam.metaengine: => set tag name: true

digikam.metaengine: => set tag type: true

digikam.metaengine: => set area struct: true

digikam.metaengine: => set xpos: true

digikam.metaengine: => set ypos: true

digikam.metaengine: => set width: true

digikam.metaengine: => set heigh: true

digikam.metaengine: => set unit: true

digikam.general: Writing tags

digikam.general: -------------------------- New Keywords ("XXX", "XXX", "XXX", "XXX", "XXX", "XXX", "Name tag", "XXX", "XXX")

digikam.metaengine: "/Pictures/2020/11/IMG_8934.jpg" ==> New Iptc Keywords: ("Name Tag", "XXX", "XXX", "XXX", "XXX", "XXX", "XXX", "XXX", "XXX")

digikam.metaengine: MetaEngine::metadataWritingMode 3

digikam.metaengine: Will write Metadata to file "/Pictures/2020/11/IMG_8934.jpg"

digikam.metaengine: wroteComment: true

digikam.metaengine: wroteEXIF: true

digikam.metaengine: wroteIPTC: true

digikam.metaengine: wroteXMP: true

The X11 connection broke (error 1). Did the X11 server die?

KCrash: Application 'digikam' crashing...

KCrash: Attempting to start /usr/libexec/drkonqi

digikam.metaengine: File time stamp restored

digikam.metaengine: Metadata for file "IMG_8934.jpg" written to file.

ASSERT failure in Q_GLOBAL_STATIC: "The global static was used after being destroyed", file /usr/include/qt5/QtCore/qglobalstatic.h, line 139

Unable to start Dr. Konqi

Rob

[Maik Qualmann]

See here:

https://bugs.kde.org/show_bug.cgi?id=429335

Maik

Am Donnerstag, 19. November 2020, 18:36:41 CET schrieb Rob Dueckman:

> Digikam has started to crash when tagging faces.  This started after
> building from sources pulled after Maik's file download fixes, but lots
> of other changes were pushed around the same time.
>
> When a face is recognized, if you apply the face tag to the image, DK
> immediately crashes.
>
> Here is the log output: (TAGs have been XXX'd for privacy)
>
> qt.text.layout: do layout
> qt.accessibility.cache: insert - id: 2147485717 iface:
> QAccessibleInterface(0x14ab10e0 name="Applying face changes"
> role=StaticText obj=QLabel(0x17f82bf0)"invisible")
> digikam.general: Found FacesEngine identity 2568 for tag 4310
> qt.accessibility.cache: insert - id: 2147485718 iface:
> QAccessibleInterface(0x1046d980 name="Assigning image tags"
> role=StaticText obj=QLabel(0x17f381e0)"invisible")
> digikam.general: Scheduled to write
> digikam.general: No write to baloo
> +++++++++++++++++++++++++++++++++++++
> qt.accessibility.cache: insert - id: 2147485719 iface:
> QAccessibleInterface(0x103fe940 name="Writing metadata to files"
> role=StaticText obj=QLabel(0x12a2c770)"invisible")
> digikam.metaengine: Set face region: 0.435847 0.360615 0.301257
> 0.230655
> digikam.metaengine: => set tag name: true
> digikam.metaengine: => set tag type: true
> digikam.metaengine: => set area struct: true
> digikam.metaengine: => set xpos: true
> digikam.metaengine: => set ypos: true
> digikam.metaengine: => set width: true
> digikam.metaengine: => set heigh: true
> digikam.metaengine: => set unit: true
> digikam.general: Writing tags
> digikam.general: -------------------------- New Keywords ("XXX", "XXX",
> "XXX", "XXX", "XXX", "XXX", "Name tag", "XXX", "XXX")
> digikam.metaengine: "/Pictures/2020/11/IMG_8934.jpg" ==> New Iptc
> Keywords: ("Name Tag", "XXX", "XXX", "XXX", "XXX", "XXX", "XXX", "XXX",
> "XXX")
> digikam.metaengine: MetaEngine::metadataWritingMode 3
> digikam.metaengine: Will write Metadata to file
> "/Pictures/2020/11/IMG_8934.jpg"
> digikam.metaengine: wroteComment: true
> digikam.metaengine: wroteEXIF: true
> digikam.metaengine: wroteIPTC: true
> digikam.metaengine: wroteXMP: true
> The X11 connection broke (error 1). Did the X11 server die?
> KCrash: Application 'digikam' crashing...
> KCrash: Attempting to start /usr/libexec/drkonqi
> digikam.metaengine: File time stamp restored
> digikam.metaengine: Metadata for file "IMG_8934.jpg" written to file.
> ASSERT failure in Q_GLOBAL_STATIC: "The global static was used after
> being destroyed", file /usr/include/qt5/QtCore/qglobalstatic.h, line
> 139
> Unable to start Dr. Konqi
>
>
> Rob