Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include exiv2lib_export.h where needed and cleanup headers #640

Merged
merged 2 commits into from Jan 11, 2019

Conversation

Projects
None yet
4 participants
@piponazo
Copy link
Collaborator

piponazo commented Jan 7, 2019

This PR fixes #636.

Furthermore I took the opportunity to cleanup the useless inclusions in the public headers of Exiv2. This might look like a scary change, but the CI jobs are building in all the platforms and many linux distributions.

Nonetheless, I decided to add @cgilles in the list of reviewers to check if he might see any problem with Exiv2 consumers such as Digikam.

@piponazo piponazo requested review from clanmills , D4N and cgilles Jan 7, 2019

@piponazo piponazo force-pushed the IncludeExportFile_0.27 branch from 4b7713f to 7f59ef1 Jan 7, 2019

@codecov

This comment has been minimized.

Copy link

codecov bot commented Jan 7, 2019

Codecov Report

Merging #640 into 0.27 will decrease coverage by 0.35%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             0.27     #640      +/-   ##
==========================================
- Coverage   62.78%   62.43%   -0.36%     
==========================================
  Files         154      154              
  Lines       20702    20524     -178     
==========================================
- Hits        12998    12814     -184     
- Misses       7704     7710       +6
Impacted Files Coverage Δ
include/exiv2/jpgimage.hpp 100% <ø> (ø) ⬆️
include/exiv2/metadatum.hpp 100% <ø> (ø) ⬆️
include/exiv2/asfvideo.hpp 0% <ø> (ø) ⬆️
include/exiv2/epsimage.hpp 0% <ø> (ø) ⬆️
include/exiv2/types.hpp 100% <ø> (+3.84%) ⬆️
include/exiv2/futils.hpp 100% <ø> (ø) ⬆️
include/exiv2/quicktimevideo.hpp 0% <ø> (ø) ⬆️
src/bigtiffimage.cpp 62.03% <ø> (ø) ⬆️
include/exiv2/psdimage.hpp 100% <ø> (ø) ⬆️
src/matroskavideo.cpp 0% <ø> (ø) ⬆️
... and 47 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95fd61b...6f8d8e3. Read the comment docs.

@clanmills
Copy link
Collaborator

clanmills left a comment

@piponazo Can you give me a "high level" view of this change.

Around Christmas 2014, Thomas B and I discussed the headers which were confused and jumbled. We adopted the model that application code (samples and user code) should use: #include <exiv2/exiv2.hpp> and nothing else. All library code would use #include "config.h" followed by #include "exif.hpp" etc. Library code would never use #include <exiv2/anything> to avoid reading files from isystem.

I'm not resisting this change. However, I'd like to understand your include file model/concept.

I'd also like confirmation from @mardy that the issue reported (#636) can be be solved by following the #include <exiv2/exiv2.hpp> model discussed above.

@piponazo

This comment has been minimized.

Copy link
Collaborator Author

piponazo commented Jan 7, 2019

Sure. IMHO it is fine to recommend Exiv2 clients to just use <exiv2/exiv2.hpp>, however you never know how they will consume the library. I am also one of those c++ developers which try to be minimalist with the header inclusions, and I just include what I need.

In the first commit I did not remove anything from <exiv2/exiv2.hpp> but I just made sure that the file which is automatically generated by CMake (exiv2lib_export.h) is included where needed. Note that this file contains the macros used to export/import the Exiv2 symbols.

In the second commit, I cleaned up some of the header inclusions inclusions that were not needed. I have seen this issue practically in every project in which I have participated ... people do not put too much attention in the header inclusions. Copy & pasting header inclusions seems to be something very common, and we end up having more header inclusions than needed.

I could do another iteration in this branch and make sure that we are always adding config.h in the library code (which seems not to be the case right now).

@clanmills

This comment has been minimized.

Copy link
Collaborator

clanmills commented Jan 7, 2019

As long as the #include <exiv2/exiv2.hpp> concept works, I'm not bothered about what's on the inside. We should discourage users from including a subset of headers of their choosing.

We haven't heard back from @mardy. If using #include <exiv2/exiv2.hpp> make his issue disappears, that's a strong endorsement for the design that Thomas and I discussed. We have the freedom to change stuff "under the hood" without worrying about breaking user's code.

I'm going to approve this and thank you for working on this.

@clanmills

This comment has been minimized.

Copy link
Collaborator

clanmills commented Jan 7, 2019

You're right that cut'n'pasting includes is common. It's quick and works at the time. However it's not good style. As you have observed, the compiler is uselessly re-reading lots of irrelevant code. We could investigate using pre-compiled headers, however exactly how these behave depends on the compiler.

By using the #include <exiv2/exiv2.hpp> model, we are telling users "Don't mix your own headers". We should have #include "config.h" as the first include of every library C++ file. And we should avoid #include "foo.hpp" inside library include files.

It's not a perfect world and we don't always follow our own rules.

However, let's always encourage users to use #include <exiv2/exiv2.hpp>. Gilles reported something a couple of years ago concerning header files. I asked him to only use #include <exiv2/exiv2.hpp> and the issue went away. And didn't return.

@mardy

This comment has been minimized.

Copy link

mardy commented Jan 7, 2019

Hi @clanmills! Yes, #include <exiv2/exiv2.hpp> works, but I would argue that this by itself is not an endorsement for that approach. It's a certainly valid approach, and very much used in the Glib world, however it's generally frawned upon in C++ projects, because of compilation speed reasons. On the other hand, I cannot tell how much these reasons are actual nowadays. :-)

@clanmills

This comment has been minimized.

Copy link
Collaborator

clanmills commented Jan 7, 2019

Thanks, @mardy for responding. You can see that @piponazo is sympathetic to your point of view and is polishing the include files.

It's a good thing to review this, the last time I went over that was with Thomas at Christmas 2014. I remember it well, because I was in Tenerife for Christmas vacation with a friend from Helsinki.

So, everything ends well here. Thank You for reporting this. And a huge thank you to @piponazo for doing something about this. A round of applause for Luis. Clap-Clap-Clap. Thank You.

@D4N

This comment has been minimized.

Copy link
Contributor

D4N commented Jan 7, 2019

On the other hand, I cannot tell how much these reasons are actual nowadays. :-)

These issues are more present than ever in the days of template metaprograming. Furtunately, we don't use a lot of that, so including all of exiv2 should be relatively light (I guess).

@D4N

D4N approved these changes Jan 7, 2019

Copy link
Contributor

D4N left a comment

Since this removes unused includes and does not break the build, it's fine by me!

Let's wait what @cgilles has to say, too.

@piponazo piponazo referenced this pull request Jan 11, 2019

Closed

Fix644 on 0.27 #647

@piponazo piponazo force-pushed the IncludeExportFile_0.27 branch from 7f59ef1 to 6f8d8e3 Jan 11, 2019

@piponazo piponazo merged commit e07326f into 0.27 Jan 11, 2019

6 of 7 checks passed

codecov/project 62.43% (-0.36%) compared to 95fd61b
Details
ci/gitlab/IncludeExportFile_0.27 Pipeline passed on GitLab
Details
codecov/patch Coverage not affected when comparing 95fd61b...6f8d8e3
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@piponazo piponazo deleted the IncludeExportFile_0.27 branch Jan 11, 2019

@piponazo piponazo referenced this pull request Jan 11, 2019

Merged

Include export file #650

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.