Differential D16163

refactor taglibextractor to functions specific for metadata type
ClosedPublic
Actions

Authored by astippich on Oct 12 2018, 5:29 PM.

Details

Reviewers
bruns
Commits
R286:5836ca4adefd: refactor taglibextractor to functions specific for metadata type
Summary

Refactor the specific extraction funtions in taglibextractor
so that they can be re-used for files of different mimetype,
but with the same tag types for their metadata.
No functional change intended.

Diff Detail

Repository
R286 KFileMetaData
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
astippich created this revision.Oct 12 2018, 5:29 PM
Restricted Application added projects: Frameworks, Baloo. · View Herald TranscriptOct 12 2018, 5:29 PM
Restricted Application added subscribers: Baloo, kde-frameworks-devel. · View Herald Transcript
astippich requested review of this revision.Oct 12 2018, 5:29 PM
astippich added a comment.Oct 12 2018, 5:29 PM

To be on the safe side here: I am allowed to modify private member functions regarding binary compatibility, right?

mgallien added a subscriber: mgallien.Oct 12 2018, 5:46 PM
In D16163#342046, @astippich wrote:

To be on the safe side here: I am allowed to modify private member functions regarding binary compatibility, right?

This page is a very good reference: https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B

bruns added a comment.Oct 12 2018, 5:56 PM
In D16163#342046, @astippich wrote:

To be on the safe side here: I am allowed to modify private member functions regarding binary compatibility, right?

Non-virtual methods do not affect the class layout or the vtable layout, so you are safe here for sure. private or not does not matter.

bruns added inline comments.Oct 12 2018, 5:59 PM
src/extractors/taglibextractor.h
27

Keep the includes in the implementation and just do a forward declaration of the types (possible as you only use these as pointer types in the header).

astippich added a comment.Oct 12 2018, 6:32 PM
In D16163#342057, @mgallien wrote:
In D16163#342046, @astippich wrote:

To be on the safe side here: I am allowed to modify private member functions regarding binary compatibility, right?

This page is a very good reference: https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B

Thanks, I already knew that side, but I tend to ask explicitly in case I misunderstood something. I really don't want to mess up frameworks :)

In D16163#342061, @bruns wrote:
In D16163#342046, @astippich wrote:

To be on the safe side here: I am allowed to modify private member functions regarding binary compatibility, right?

Non-virtual methods do not affect the class layout or the vtable layout, so you are safe here for sure. private or not does not matter.

Thanks for the explanation!

astippich updated this revision to Diff 43500.Oct 12 2018, 6:53 PM
  • use forward declaration
bruns accepted this revision.Oct 12 2018, 8:12 PM
bruns added inline comments.
src/extractors/taglibextractor.h
44

can also be written as

namespace TagLib::ID3v2 {
    class Tag;
}
namespace TagLib::MP4 {
    class Tag;
}
...

a little bit shorter, but otherwise just a matter of style preference ...

This revision is now accepted and ready to land.Oct 12 2018, 8:12 PM
Closed by commit R286:5836ca4adefd: refactor taglibextractor to functions specific for metadata type (authored by astippich). · Explain WhyOct 13 2018, 8:48 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 43524

View Options

src/extractors/taglibextractor.h

Loading...
View Options

src/extractors/taglibextractor.cpp

Loading...