search

Exiv2 / exiv2

Image metadata library and tools
http://www.exiv2.org/
Other
927 stars 282 forks source link

C++ Modernisation for Exiv2 v1.00 #1557

Open clanmills opened 3 years ago

clanmills commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 https://github.com/Exiv2/exiv2/issues/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 commented 3 years ago

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 commented 3 years ago

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

1div0 commented 3 years ago

Hi++ *

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

Please have a look at the GCC11.log

Thanks.

clanmills commented 3 years ago

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 commented 3 years ago

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 commented 1 year ago

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

1div0 commented 1 year ago

@neheb I'm okay with that.