Skip to content
This repository has been archived by the owner on Jan 27, 2020. It is now read-only.

C++17 rewrite #14

Merged
merged 10 commits into from
Oct 25, 2019
Merged

C++17 rewrite #14

merged 10 commits into from
Oct 25, 2019

Conversation

PeterMatula
Copy link
Collaborator

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 avast/retdec#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 PeterMatula requested review from metthal and s3rvac October 23, 2019 12:29
Copy link
Member

@s3rvac s3rvac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I have included two remarks to consider.

README.md Show resolved Hide resolved
include/pelib/ImportDirectory.h Show resolved Hide resolved
@PeterMatula
Copy link
Collaborator Author

PeterMatula commented Oct 24, 2019

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
Copy link
Member

s3rvac commented Oct 24, 2019

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
Copy link
Collaborator Author

PeterMatula commented Oct 24, 2019

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

This simplifies the code and allows us to use more readable lambdas than
std::bind().
@PeterMatula
Copy link
Collaborator Author

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.

@PeterMatula PeterMatula merged commit a546984 into master Oct 25, 2019
@PeterMatula PeterMatula deleted the c++17 branch October 25, 2019 17:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants