avast / pelib

PE file manipulation library.
MIT License
63 stars 30 forks source link

C++17 rewrite #14

Closed PeterMatula closed 5 years ago

PeterMatula commented 5 years ago

C++17 deprecated and removed some language features. This made this project not compile inside RetDec.

I replaced these features and bumped Travis and Appveyor environments. I also bumped stuff in README.md and CHANGELOG.md.

Now it compiles without warnings or errors.

Since PeLib does not have its own unit or regression tests, can one (both?) of you (@s3rvac, @metthal) please look at it? I was careful, but it is easy to mess up those lambdas :-)

I will need this for https://github.com/avast/retdec/issues/650. But for now, I will use reference to this branch (instead of master) in RetDec, so it is not a total blocker. It will also run RetDec tests with these changes, so it may catch some problems before we merge here.

PeterMatula commented 5 years ago

LOL 651987a

I thought there is something fishy about this when I wrote it. But it "compiled" so I thought ok, probably some partial application is going on in there - that method has one argument but was called with zero. It turns out, it did not compile when used in RetDec - templates :-) I guess no partial application in C++. Is it ok now? That comparator object description for std::max_element() is non-trivial.

s3rvac commented 5 years ago

Is it ok now? That comparator object description for std::max_element() is non-trivial.

The comparison function should take two arguments and return true when the first one is smaller than the second one. The current code is

[](const auto& i1, const auto& i2) { return i1.biggerFileOffset(i2); }

Member function biggerFileOffset() is

bool PELIB_IMAGE_SECTION_HEADER::biggerFileOffset(const PELIB_IMAGE_SECTION_HEADER& ish) const
{
    return PointerToRawData < ish.PointerToRawData;
}

So, the comparator is correct.

As for the compilation, when a compiler encounters a template, it first only checks its syntax. It checks its semantic only after the template is instantiated with particular types/values.

PeterMatula commented 5 years ago

Thanks. I will merge this once RetDec tests that use this pass.

PeterMatula commented 5 years ago

Everything looks good now, but I will still wait for RetDec regression tests to pass witch this branch - I'm currently running this code inside RetDec's issue-650 (C++17 support) branch and there are some more complications in there - likely unrelated to changes here.