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

Fixed RETDEC-74 and RETDEC-61 #1003

Merged
merged 8 commits into from
Aug 24, 2021
Merged

Fixed RETDEC-74 and RETDEC-61 #1003

merged 8 commits into from
Aug 24, 2021

Conversation

ladislav-zezula
Copy link
Contributor

No description provided.

@ladislav-zezula
Copy link
Contributor Author

let's run TC tests

@ladislav-zezula
Copy link
Contributor Author

let's run TC tests

@ladislav-zezula
Copy link
Contributor Author

let's run TC tests

@PeterMatula PeterMatula self-requested a review August 23, 2021 12:21
Copy link
Collaborator

@PeterMatula PeterMatula left a comment

Choose a reason for hiding this comment

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

This is too complex for me to actually review in detail. So if the tests are passing (I fixed doxygen) then so be it...

I went over it as much as I could and have only one question in the comments below, other than that I would merge it as it is.

@@ -319,11 +319,11 @@ void MpressPlugin::fixImportsAndEp(DynamicBuffer& buffer)
readPos++; // skip null terminator

// Set FirstThunk to point into IAT
int fileIndex = _peFile->impDir().getFileIndex(moduleName, PeLib::NEWDIR);
int fileIndex = _peFile->impDir().getFileIndex(moduleName, true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The original currdir enum maybe wasn't the prettiest, but I still think the code:

getFileIndex(moduleName, PeLib::NEWDIR)
getFileIndex(moduleName, PeLib::OLDDIR)

was more telling then:

getFileIndex(moduleName, true)
getFileIndex(moduleName, false)

Why was this changed? Do you just like it better, or was there some other reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A presence of an enum indicated that it could be more values, whilst it's only decision between an old import directory or a new one. Hence I changed it to true vs false.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In this case, I think the enum could have just served to make the code more human-readable, without any implications/expectations on how many values there are.

Copy link
Collaborator

Choose a reason for hiding this comment

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

However, this might be just my personal preference and if you like it as it is now, ok. Don't refactor it back unless you think it would be better.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @PeterMatula here. When i see this code what does true really mean? When I see NEWDIR it gives me at least some sort of semantics of what is going on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@metthal metthal Aug 24, 2021

Choose a reason for hiding this comment

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

You are missing the point. I don't want to read the signature of every single function when I'm trying to understand the code. Imagine we need additional functionality which needs another flag. What is better? Having getFileIndex(moduleName, true, false)? Wwhat does each of them mean? I don't know. So I check the signature. I come back tomorrow and I don't remember which one is which so I need to check the signature again. The best thing would even be if those flags were strongly typed (enum class) because if I want to call this function I need to exactly know which bool is which and compiler won't help me if I make a mistake. If you turn all your bool arguments into strongly-typed arguments then you have the compiler to save you when you by accident pass the wrong value into the parameter. Overall, bool arguments are bad.

Copy link
Contributor Author

@ladislav-zezula ladislav-zezula Aug 24, 2021

Choose a reason for hiding this comment

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

Surely we could discuss coding rules ad libitum.

Do you want me to return the enum?

@PeterMatula
Copy link
Collaborator

I have more questions in the related tests PR - avast/retdec-regression-tests#107
If you were to write long responses on GitHub, lets just have a meet on Zoom.

@ladislav-zezula
Copy link
Contributor Author

let's run TC tests

@PeterMatula PeterMatula merged commit 9dd86ac into master Aug 24, 2021
PeterMatula added a commit that referenced this pull request Aug 24, 2021
@ladislav-zezula ladislav-zezula deleted the LZ_RETDEC_74 branch November 2, 2022 18:20
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

Successfully merging this pull request may close these issues.

3 participants