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

Token locations python bindings #120

Merged
merged 8 commits into from
Aug 11, 2020

Conversation

MatejKastak
Copy link
Member

@MatejKastak MatejKastak commented Aug 4, 2020

Add a way to access locations of various objects in yara files from python.

Interface

  • location is now represented as a structure instead of std::pair
  • keeping the old Rule::Location interface, line_number and file_path
  • exposing TokenType for each Token
  • extending the String interface to provide location attribute just like the rules
  • exposing some tokens linked to symbols
    • Rule - token_first, token_last
    • String - token_first, token_last, token_id, token_assign

TODO

  • 5e6dc9c contains changes that convert TokenType to cpp11 style enum class enums, I reverted them because I think they are out of the scope of this PR, if you want I can open a separate PR for just this. What do you think about this change?
  • I provided a baseline for the tests, but currently some of the token locations are not correct. I have marked them with FIXME: comments. Keep in mind that the values in the tests are my expectations, I you think they are not right we can discuss the changes.

@MatejKastak MatejKastak force-pushed the token_locations branch 4 times, most recently from 7f1a4f4 to ccdde60 Compare August 9, 2020 12:30
In order to preserve backwards compatibility the `line_number` property
has been kept, but will be deprecated. Now any other `Location` instance
will include `line_number`, that should not be mentioned in the documentation.
@TadeasKucera
Copy link
Contributor

First of all thank you for the PR and for recognizing the problems with location of some tokens. The code is alright and I approve it. As for the first checkbox: Please do open a separate pull request for it.

Copy link
Contributor

@TadeasKucera TadeasKucera 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 will soon fix the location computing and uncomment the tests. Thank you!

@TadeasKucera TadeasKucera merged commit eee73e8 into avast:master Aug 11, 2020
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

3 participants