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

C++ Modernisation for Exiv2 v1.00 #1557

Open
clanmills opened this issue Apr 15, 2021 · 10 comments
Open

C++ Modernisation for Exiv2 v1.00 #1557

clanmills opened this issue Apr 15, 2021 · 10 comments
Assignees
Milestone

Comments

@clanmills
Copy link
Collaborator

The code should be "modernised" for v1.00. The priority feature is to drop support for C++98 and replace auto_ptr with unique_ptr.

In 2018, KDE requested C++11 and it is desirable to aim for C++11 and newer. If retaining C++11 is a cause of pain, it should be discussed with @alexvanderberkel as he has a working relationship with KDE.

Please ensure the code builds and runs on C++11, 14, 17 and today's C++20 compilers. GCC, Clang and Microsoft CL.

It's likely that parts of the code could be simplified using modern C++ features. It's fine to do some optimisation, however that's not an objective of this project.

A lot of work was performed by @neheb Rosen on the 'master' branch using clang-tidy and clang-format. He has been invited to join the Exiv2 v1.00 project to ensure his valuable contribution ships with v1.00.

Parts of the code were deprecated in 0.27 and compiler warning generated. These parts of the code deal with support for EPS, SSH and Video. Deprecated code should be removed from v1.00.

Finally, we should consider using clang-format on all the code in Exiv2 v1.00. The advantage of not doing this is to facilitate back-porting fixes to 0.27-maintenance. We could consider doing this early in the v1.00 project to maximise the time for adverse consequences to appear. I think the correct time to do this is immediately following "Code Complete" and before "RC1" (early October 2021).

@vog
Copy link
Contributor

vog commented Apr 19, 2021

Regarding:

Parts of the code were deprecated in 0.27 and compiler warning generated. These parts of the code deal with support for EPS, SSH and Video. Deprecated code should be removed from v1.00.

It seems that I missed the part about EPS. What exactly are the objectives with EPS support? I'm asking because I know of at least one application that makes active use of EPS metadata and that is still in active use. Such an application one would be stuck with Exiv2 0.27 if EPS support was removed in 1.00.

What exactly is needed to make EPS support survive 1.00?

@rentapacs
Copy link

Hi Robin, all,
since @vog alerted me of the pending removal of EPS support from Exiv2 1.00 I'd like to chime in here and speak out for keeping it in the code. My company develops and supports a software system which heavily relies on the ability to read and write image metadata in IPTC/XMP formats to and from various image formats. Since our system lives in a professional publishing environment image formats like PSD and EPS are mandatory and must be supported. That was the main reason for @vog and me to contribute Exiv2 patches for the JPEG and PSD formats and provide a complete implementation to read/write XMP for the EPS format. This happened roughly 10 years ago but Exiv2 has done a perfect job since. Please reconsider the EPS removal and let us know if we can help to bring the code into shape to be acceptable for 1.0.

Congratulations and thanks for the IMaEA which I discovered recently and made my day especially the "meta" topics in chapter 11 ... so true!

Thanks @clanmills and all contributors for keeping Exiv2 alive over the years!

Best from Berlin ... Michael

@piponazo
Copy link
Collaborator

Hi @rentapacs & @vog,

For me it is fine to recover in the main branch the EPS support. I just removed it because it was marked as deprecated after the discussion we had a long time ago here #603 . However, if you are using EPS and are happy with its current implementation, I'll happily recover that code.

I hope others have no objections about it 😉

@clanmills
Copy link
Collaborator Author

I'm happy with everything discussed here. I don't recall any issues with the EPS code. Thank You @vog and @rentapacs for your contribution. And to @piponazo for closing this.

Lots of great work by everybody. Thanks.

@vog
Copy link
Contributor

vog commented Apr 27, 2021

@piponazo @clanmills Thanks a lot for restoring EPS support!

@1div0
Copy link
Collaborator

1div0 commented Apr 29, 2021

Hi++ *

Just tried to compile latest code from main branch with -fanalyzer

Please have a look at the GCC11.log

Thanks.

@clanmills
Copy link
Collaborator Author

clanmills commented May 23, 2021

Hi @1div0. I've looked at your log. I think it built correctly. I've never used the option -fanalyzer and it's not supported by the clang compiler on macOS.

I've installed GCC 11.1 on Ubuntu and can't it to build anything with/without that option.

If it built OK for you (and passed the test suite), I'm happy. If there is something to be investigated, can you open a new issue?

With -fanalyzer:

$ cmake .. -DEXIV2_ENABLE_BMFF=On -DCMAKE_CXX_FLAGS=-fanalyzer -DCMAKE-CXX-COMPILER=$(which gcc-11)
...
    clang: error: unknown argument '-fanalyzer'; did you mean '-Xanalyzer'?
...

Vanilla build (exiv2 v0.27.4.3)

$ cmake .. -DEXIV2_ENABLE_BMFF=On -DCMAKE-CXX-COMPILER=$(which gcc-11) -DCMAKE_CXX_STANDARD=98
...
    /usr/bin/ld: cannot find -lstdc++
...

Clearly GCC-11 isn't correctly installed and I don't think it's worth solving the installation issue.

@neheb
Copy link
Collaborator

neheb commented Jun 21, 2021

Hi++ *

Just tried to compile latest code from main branch with -fanalyzer

Please have a look at the GCC11.log

Thanks.

I'd look into coverity and clang-analyzer before fanalyzer. It seems to have a lot of noise.

@neheb
Copy link
Collaborator

neheb commented Feb 19, 2023

I assume this can be closed as main uses C++17 with some C++20

@1div0
Copy link
Collaborator

1div0 commented Feb 20, 2023

@neheb I'm okay with that.

@kevinbackhouse kevinbackhouse modified the milestones: v0.28.0, Backlog Nov 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants