Remove dead code + Move samples #412

piponazo · 2018-08-24T16:40:25Z

In this PR I did the following changes:

remove some test files under the src folder that were not being compiled.
Move some test applications to the samples folder and compile them. Previously these test applications were not being compiled.
Run clang-format in the files moved to the samples folder.

codecov · 2018-08-24T16:54:10Z

Codecov Report

Merging #412 into master will increase coverage by 0.13%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #412      +/-   ##
==========================================
+ Coverage   53.93%   54.06%   +0.13%     
==========================================
  Files         173      172       -1     
  Lines       26657    26592      -65     
==========================================
  Hits        14378    14378              
+ Misses      12279    12214      -65

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 ad00449...4f3a02e. Read the comment docs.

D4N · 2018-08-25T07:21:06Z

samples/crwedit.cpp


 void remove(Exiv2::Internal::CiffHeader* pHead);
 void add(Exiv2::Internal::CiffHeader* pHead);
 void help();
 void write(const std::string& filename, const Exiv2::Internal::CiffHeader* pHead);

-int main(int argc, char* const argv[])
-try {
+int main(int argc, char* const argv[]) try {


[Suggestion] Wrap the try {} catch {} block in braces.

And maybe for the other files too.

Sure, I will add it 👍

D4N · 2018-08-25T07:50:00Z

Concerning 4f3a02e: I am not sure whether these are really useless. At least tiff-test.cpp seems to be doing stuff with exiv2-empty.jpg which the testsuite uses.

@clanmills Do you know what these files do?

piponazo · 2018-08-25T08:26:11Z

For sure it was dead code, since we were not compiling those files. Unless @clanmills know something more about that file, and he thinks it is useful somehow, I would vote to test that part of the code when somebody revisits it.

clanmills · 2018-08-25T09:13:44Z

Good find. I don't know anything about this code.

codecov · 2018-08-25T11:55:57Z

Codecov Report

Merging #412 into master will increase coverage by 0.13%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #412      +/-   ##
==========================================
+ Coverage   53.93%   54.06%   +0.13%     
==========================================
  Files         173      172       -1     
  Lines       26657    26592      -65     
==========================================
  Hits        14378    14378              
+ Misses      12279    12214      -65

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 ad00449...d269d5e. Read the comment docs.

D4N · 2018-08-25T15:48:04Z

Looks like stuff that Andreas added ages ago for testing. Let's get rid of it. I'll approve this later, when I'm not on mobile.

clanmills · 2018-09-02T04:03:40Z

The utilities crwedit.cpp and crwparse.cpp are breaking the build on MinGW. For the moment, I've commented them off in samples/CMakeLists.txt

There are two issues.

crwimage_int.cpp requires timegm()
We can easily fix this by updating src/timegm.h (code below)
crwedit.cpp has unsatisfied external such as Exiv2::Internal::CiffHeader::read
I think we should remove crwedit.cpp from the code base (and samples/CMakeLists.txt).

Analysis

crwimage_int.cpp requires timegm()

[ 48%] Linking CXX shared library ../bin/libexiv2.dll
CMakeFiles/exiv2lib.dir/objects.a(crwimage_int.cpp.obj):crwimage_int.cpp:(.text+0x4b11): undefined reference to `timegm'
collect2.exe: error: ld returned 1 exit status
make[2]: *** [src/CMakeFiles/exiv2lib.dir/build.make:821: bin/libexiv2.dll] Error 1
make[1]: *** [CMakeFiles/Makefile2:249: src/CMakeFiles/exiv2lib.dir/all] Error 2
make: *** [Makefile:130: all] Error 2
677 rmills@rmillsmm-w7:~/gnu/github/exiv2/exiv2/build $

This can be fixed with a simpler/revised src/timegm.h

#pragma once
#include <time.h>

// The UTC version of mktime
/* timegm is replaced with _mkgmtime on Windows (msvc && mingw) */
#if defined(__MINGW__) || defined(_MSC_VER)
#define timegm _mkgmtime
#endif

crwedit.cpp has unsatisfied external such as Exiv2::Internal::CiffHeader::read

I'm a little lost on this one. These entries are not in the library. I think we should remove crwedit.cpp from the code base.

Scanning dependencies of target crwedit
[ 69%] Building CXX object samples/CMakeFiles/crwedit.dir/crwedit.cpp.obj
[ 69%] Linking CXX executable ../bin/crwedit.exe
CMakeFiles/crwedit.dir/objects.a(crwedit.cpp.obj):crwedit.cpp:(.text+0x31a): undefined reference to `Exiv2::Internal::CiffHeader::read(unsigned char const*, unsigned int)'
CMakeFiles/crwedit.dir/objects.a(crwedit.cpp.obj):crwedit.cpp:(.text+0x3dd): undefined reference to `Exiv2::Internal::CiffHeader::print(std::ostream&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) const'
CMakeFiles/crwedit.dir/objects.a(crwedit.cpp.obj):crwedit.cpp:(.text+0x668): undefined reference to `Exiv2::Internal::CiffHeader::write(std::vector<unsigned char, std::allocator<unsigned char> >&) const'
CMakeFiles/crwedit.dir/objects.a(crwedit.cpp.obj):crwedit.cpp:(.text+0xa22): undefined reference to `Exiv2::Internal::CiffHeader::remove(unsigned short, unsigned short)'
CMakeFiles/crwedit.dir/objects.a(crwedit.cpp.obj):crwedit.cpp:(.text+0xc01): undefined reference to `Exiv2::Internal::CiffHeader::add(unsigned short, unsigned short, Exiv2::DataBuf)'
CMakeFiles/crwedit.dir/objects.a(crwedit.cpp.obj):crwedit.cpp:(.rdata$.refptr._ZTVN5Exiv28Internal10CiffHeaderE[.refptr._ZTVN5Exiv28Internal10CiffHeaderE]+0x0): undefined reference to `vtable for Exiv2::Internal::CiffHeader'
collect2.exe: error: ld returned 1 exit status
make[2]: *** [samples/CMakeFiles/crwedit.dir/build.make:92: bin/crwedit.exe] Error 1
make[1]: *** [CMakeFiles/Makefile2:948: samples/CMakeFiles/crwedit.dir/all] Error 2
make: *** [Makefile:130: all] Error 2
698 rmills@rmillsmm-w7:~/gnu/github/exiv2/exiv2/build $

clanmills · 2018-09-02T15:09:15Z

The MinGW build doesn't appear to have any Exiv2::Internal entry points (and that's possibly correct). When I look for 'open', I find 10 entries on MinGW (and Mac).

When I look for 'Internal' , I find 0 (none) on MinGW and 1911 entries on Mac.

So the linker is correct, Exiv2::Internal::CiffHeader::read() is not in libexiv2.dll.a on MinGW when built with GCC 7.3.0.

550 rmills@rmillsmm-w7:~/gnu/github/exiv2/exiv2/build $ nm -g --demangle lib/libexiv2.dll.a | grep open | grep T
0000000000000000 T Exiv2::RemoteIo::isopen() const
0000000000000000 T Exiv2::FileIo::isopen() const
0000000000000000 T Exiv2::MemIo::isopen() const
0000000000000000 T Exiv2::RemoteIo::open()
0000000000000000 T Exiv2::FileIo::open()
0000000000000000 T Exiv2::FileIo::open(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)
0000000000000000 T Exiv2::MemIo::open()
0000000000000000 T Exiv2::ImageFactory::open(std::auto_ptr<Exiv2::BasicIo>)
0000000000000000 T Exiv2::ImageFactory::open(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, bool)
0000000000000000 T Exiv2::ImageFactory::open(unsigned char const*, long)
551 rmills@rmillsmm-w7:~/gnu/github/exiv2/exiv2/build $ nm -g --demangle lib/libexiv2.dll.a | grep open | grep T |wc
     10      45     687
552 rmills@rmillsmm-w7:~/gnu/github/exiv2/exiv2/build $ nm -g --demangle lib/libexiv2.dll.a | grep Internal | grep T |wc
      0       0       0
553 rmills@rmillsmm-w7:~/gnu/github/exiv2/exiv2/build $

The quick fix is to remove crwedit.cpp from the code base.

piponazo · 2018-09-02T15:42:58Z

For me it is fine to remove the app. I just moved to the samples in case it could be useful, but actually we were not compiling it before. I will create a PR to remove it.

clanmills · 2018-09-02T15:59:32Z

Thanks @piponazo

Removing it (and crwparse.cpp) is probably the right thing. And remove it from samples/CMakeLists.txt

We could create a directory contrib/obsolete and move it there. Might be useful for dealing with CR3. We don't built it of course - just add contrib/obsolete/ReadMe.txt to say something about the code.

piponazo · 2018-09-02T16:05:21Z

I do not like the idea of leaving code around "just-in-case ...". But we could create such text file mentioning obsolete pieces of code that were deleted and could be useful in the future for addressing some refactoring. In such document we could point to the PR deleting the code, so that a future contributor could see why the code was deleted and be able to recover the code from the git history.

clanmills · 2018-09-02T16:11:18Z

Let's just remove it. When the time comes to deal with CR3 (and the like), we'll probably write new utilities anyway. I had the SVN 'team' repos for this purpose. There's no reason to retain obsolete code in the public code-base.

piponazo added 6 commits August 24, 2018 16:55

Remove utiltest.cpp that is not compiled

7318723

Move crwedit from exiv2lib to samples

1546587

Move crwparse to samples

aa1ef56

Move xmpdump to samples

8cab691

Move mrwthumb to samples

98d3326

Remove more dead code

4f3a02e

piponazo requested review from clanmills and D4N August 24, 2018 16:40

D4N reviewed Aug 25, 2018

View reviewed changes

Add braces for the main functions in the new source files at samples

d269d5e

D4N approved these changes Aug 25, 2018

View reviewed changes

piponazo merged commit d96b619 into Exiv2:master Aug 26, 2018

piponazo deleted the removeDeadCode branch August 26, 2018 07:23

piponazo self-assigned this Sep 2, 2018

piponazo mentioned this pull request Sep 2, 2018

Remove crw samples + Fix build with MinGW #431

Merged

clanmills mentioned this pull request Sep 3, 2018

MinGW / MSYS2 build fails for v0.26 with gcc 8.2.0 #421

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove dead code + Move samples #412

Remove dead code + Move samples #412

piponazo commented Aug 24, 2018

codecov bot commented Aug 24, 2018 •

edited

Loading

D4N Aug 25, 2018

D4N Aug 25, 2018

piponazo Aug 25, 2018

D4N commented Aug 25, 2018

piponazo commented Aug 25, 2018

clanmills commented Aug 25, 2018

codecov bot commented Aug 25, 2018 •

edited

Loading

D4N commented Aug 25, 2018

clanmills commented Sep 2, 2018

clanmills commented Sep 2, 2018

piponazo commented Sep 2, 2018

clanmills commented Sep 2, 2018

piponazo commented Sep 2, 2018

clanmills commented Sep 2, 2018

Remove dead code + Move samples #412

Remove dead code + Move samples #412

Conversation

piponazo commented Aug 24, 2018

codecov bot commented Aug 24, 2018 • edited Loading

Codecov Report

D4N Aug 25, 2018

Choose a reason for hiding this comment

D4N Aug 25, 2018

Choose a reason for hiding this comment

piponazo Aug 25, 2018

Choose a reason for hiding this comment

D4N commented Aug 25, 2018

piponazo commented Aug 25, 2018

clanmills commented Aug 25, 2018

codecov bot commented Aug 25, 2018 • edited Loading

Codecov Report

D4N commented Aug 25, 2018

clanmills commented Sep 2, 2018

clanmills commented Sep 2, 2018

piponazo commented Sep 2, 2018

clanmills commented Sep 2, 2018

piponazo commented Sep 2, 2018

clanmills commented Sep 2, 2018

codecov bot commented Aug 24, 2018 •

edited

Loading

codecov bot commented Aug 25, 2018 •

edited

Loading