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

Change get_global_offset_from_line to also take an optional character position #201

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from

Conversation

Xenomega
Copy link
Member

@Xenomega Xenomega commented Jul 7, 2021

Currently, crytic-compile has a function to take a source file offset and convert it into line and character position numbers. To reverse this operation, get_global_offset_from_line could be used, but it only takes a line number, not a character position number.

This PR adds an optional character position parameter which can be used to specify a position on a line number which we want to obtain a source offset for.

It also renames get_global_offset_from_line to get_global_offset_from_line_and_character and get_line_from_offset into get_line_and_character_from_offset for clarity.

@Xenomega Xenomega requested a review from montyly July 7, 2021 15:26
@montyly montyly changed the base branch from master to dev July 9, 2021 09:38
Copy link
Contributor

@bohendo bohendo left a comment

Choose a reason for hiding this comment

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

I assume the get_line_and_character_from_offset and get_global_offset_from_line_and_character functions are only intended to be used by dependent python packages (e.g. slither) and not by crytic-compile itself? If so, this looks good to me.

@montyly
Copy link
Member

montyly commented Jan 6, 2023

This PR has been on hold because the breaking change might impact other projects. Slither has an open PR crytic/slither#889, but the related code has changed so it needs to be updated

Before merging this, we should check in the crytic and trailofbits orgs if other projects are affected (manticore or evm-cfg-builder maybe?)

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