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

Added option -l --list to retdec-bin2pat. #484

Merged
merged 1 commit into from
Feb 7, 2019
Merged

Conversation

astrelsky
Copy link
Contributor

@astrelsky astrelsky commented Feb 3, 2019

The added option -l --list is being used to pass the list of objects
from retdec-signature-from-library-creator.py as a json file.
Resolves #472

I am uncertain if the change in retdec-bin2pat's help output correctly
follows the Utility Conventions. If it does not, providing a proper
reference would be appreciated.

Regards,
Andrew Strelsky

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.

Thank you for the contribution. I have reviewed the code and wrote some suggestions that need to be addresses before we can merge this.

scripts/retdec-signature-from-library-creator.py Outdated Show resolved Hide resolved
scripts/retdec-signature-from-library-creator.py Outdated Show resolved Hide resolved
scripts/retdec-signature-from-library-creator.py Outdated Show resolved Hide resolved
scripts/retdec-signature-from-library-creator.py Outdated Show resolved Hide resolved
src/bin2pat/bin2pat.cpp Outdated Show resolved Hide resolved
src/bin2pat/bin2pat.cpp Outdated Show resolved Hide resolved
src/bin2pat/bin2pat.cpp Outdated Show resolved Hide resolved
src/bin2pat/bin2pat.cpp Outdated Show resolved Hide resolved
src/bin2pat/bin2pat.cpp Outdated Show resolved Hide resolved
src/bin2pat/bin2pat.cpp Outdated Show resolved Hide resolved
@astrelsky
Copy link
Contributor Author

@s3rvac Thank you for taking the time to put so much detail into reviewing this. I must be honest, when I awoke to the email notification I had an enormous grin on my face. I have not seen such a list of required changes since I took my introductory to C++ course. I will amend the commit this afternoon.

With regard to using json, I must confess that this is a case of using it because it was there. I should have followed the golden rule of 'keep it simple, stupid'. Thinking about it again, its use just adds overhead and consumes more memory than necessary. Considering that a user is unlikely to try to pass a huge list of objects manually, is it necessary to even have the option appear when the help is displayed? Do you have a suggestion for the command-line option?

@astrelsky astrelsky changed the title Added option -j --json to retdec-bin2pat. Added option -l --list to retdec-bin2pat. Feb 5, 2019
@astrelsky astrelsky force-pushed the master branch 2 times, most recently from 749ab4e to 6b55661 Compare February 5, 2019 23:47
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.

Thank you for the quick changes! I have several final suggestions. After incorporating them, I think I will be able to merge the PR 👍

src/bin2pat/bin2pat.cpp Outdated Show resolved Hide resolved
src/bin2pat/bin2pat.cpp Show resolved Hide resolved
src/bin2pat/bin2pat.cpp Outdated Show resolved Hide resolved
src/bin2pat/bin2pat.cpp Outdated Show resolved Hide resolved
The added option -l --list is being used to pass the list of objects
from retdec-signature-from-library-creator.py as a text file.
Resolves avast#472
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.

Looks good to me now 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants