Skip to content

Commit

Permalink
Delete dead code
Browse files Browse the repository at this point in the history
  • Loading branch information
@piponazo
piponazo committed Feb 4, 2022
1 parent 071e73f commit 1d6bac6
Show file tree
Hide file tree
Showing 2 changed files with 1 addition and 15 deletions.
6 changes: 1 addition & 5 deletions include/exiv2/datasets.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -237,11 +237,7 @@ namespace Exiv2 {
@throw Error if the record is not known;
*/
static uint16_t recordId(const std::string& recordName);
//! Return read-only list of built-in Envelope Record datasets
static const DataSet* envelopeRecordList();
//! Return read-only list of built-in Application2 Record datasets
static const DataSet* application2RecordList();
//! Print a list of all dataSets to output stream

static void dataSetList(std::ostream& os);

private:
Expand Down
10 changes: 0 additions & 10 deletions src/datasets.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,11 +125,6 @@ namespace Exiv2 {
"(Invalid)", false, false, 0, 0, Exiv2::unsignedShort, IptcDataSets::envelope, ""},
};

const DataSet* IptcDataSets::envelopeRecordList()
{
return envelopeRecord;
}

constexpr DataSet application2Record[] = {
{IptcDataSets::RecordVersion, "RecordVersion", N_("Record Version"),
N_("A binary number identifying the version of the Information "
Expand Down Expand Up @@ -402,11 +397,6 @@ namespace Exiv2 {
false, false, 0, 0, Exiv2::unsignedShort, IptcDataSets::application2, ""},
};

const DataSet* IptcDataSets::application2RecordList()
{
return application2Record;
}

constexpr DataSet unknownDataSet{0xffff, "Unknown dataset", N_("Unknown dataset"),
N_("Unknown dataset"),
false, true, 0, 0xffffffff, Exiv2::string,
Expand Down

6 comments on commit 1d6bac6

@mallman
Copy link
Collaborator

@mallman mallman commented on 1d6bac6 Feb 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @piponazo. Can you advise on how a client should migrate from the envelopeRecordList() and application2RecordList() methods, as I was using them in my library. Cheers.

@mallman
Copy link
Collaborator

@mallman mallman commented on 1d6bac6 Feb 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a direct replacement, but I do see an alternative strategy. In my library, I was statically allocating and populating a set of application2 and envelope datasets at first access. When I encountered an iptc record in parsing some metadata, I would look for a match in one of the record sets using record id and dataset number. Then I would get the IPTC dataset data from the matching exiv DataSet struct.

Instead, it looks like I should be using the various public static methods in the IptcDataSets class to look up dataset information by record id and dataset number. I'll try that and see how it goes. Cheers.

@mallman
Copy link
Collaborator

@mallman mallman commented on 1d6bac6 Feb 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another note. Exiv2::DataSet appears to be a public API, yet I can't find any use of it in the public API now that the envelopeRecordList() and application2RecordList() methods are gone. Is Exiv2::DataSet still intended to be public, and if it is does it make sense to add a static method that returns a known instance of Exiv2::DataSet given a record id and dataset number?

@mallman
Copy link
Collaborator

@mallman mallman commented on 1d6bac6 Feb 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So after further attempts, I can find no way to fully restore the capability lost by the removal of these two methods. For example, given a dataset number and record id, I can find name, title, description of an IPTC dataset through the current API, I can't find a way to access the mandatory property. This is present on the Exiv2::DataSet struct, but I can find no method that provides access to the instances of that struct contained in datasets.cpp.

I'm wondering, even if we don't restore the envelopeRecordList() and application2RecordList() methods, how about adding a method like

static DataSet* dataSet(uint16_t number, uint16_t recordId);

to the IptcDataSets class which returns an instance of DataSet for a known data set number and record id, and returns NULL for an unknown data set number or record id?

I'm happy to help with a PR if this sounds like a good idea to you. Cheers, and thanks for your work on this essential community project.

@piponazo
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @mallman , thanks for your report and feedback. It is very appreciated.

Honestly, when I removed that part of the code I did not consider that clients might be using those internal data structures. I wrongly assumed that this part of the code was not used. Of course any functionality exposed in a library which is used by many different clients will eventually be used. Classic rookie mistake from my side. I am sorry about that, I hope you did not loose too much time with this.

I will just create new PR reverting that commit.

It is good to see people using already the main branch and letting us know about mistakes like this one 😇

@mallman
Copy link
Collaborator

@mallman mallman commented on 1d6bac6 Feb 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will just create new PR reverting that commit.

Thank you, @piponazo.

It is good to see people using already the main branch and letting us know about mistakes like this one.

👍

Please sign in to comment.