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

Simplify the code #12

Merged
merged 6 commits into from
Sep 22, 2023
Merged

Simplify the code #12

merged 6 commits into from
Sep 22, 2023

Conversation

fzakaria
Copy link
Owner

No description provided.

sqlelf/elf.py Outdated Show resolved Hide resolved
sqlelf/elf.py Outdated Show resolved Hide resolved
sqlelf/elf.py Outdated Show resolved Hide resolved
sqlelf/elf.py Outdated Show resolved Hide resolved
sqlelf/elf.py Outdated Show resolved Hide resolved
return Generator(columns, column_access, _generator)


def coerce_section_name(name: str | None) -> str | None:
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the difference between "undefined" and None?

Copy link
Owner Author

Choose a reason for hiding this comment

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

TODO: add better explanation

# TODO(fzakaria): Better understand why is it auxiliary?
# this returns versions like GLIBC_2.2.5
"version": symbol.symbol_version.symbol_version_auxiliary.name
if symbol.symbol_version
Copy link
Contributor

Choose a reason for hiding this comment

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

move this to an if else expression on its own line above

Copy link
Owner Author

Choose a reason for hiding this comment

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

huh?

for binary in binaries:
# super important that these accessors are pulled out of the tight loop
# as they can be costly
binary_name = binary.name
Copy link
Contributor

Choose a reason for hiding this comment

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

why this assignment? why not do binary.name below, like for section.name?

Copy link
Owner Author

Choose a reason for hiding this comment

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

the comment says its in a tight loop for each section so I'm avoiding needless back and forth between the c-code when possible.
I thought I found it to affect the performance --

Consider the case of instructions that can have 700k row.
That's saving at least 1 boundary....

class Generator:
"""A generator for the virtual table SQLite module."""

columns: Sequence[str]
Copy link
Contributor

Choose a reason for hiding this comment

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

where do you use these attributes?

Copy link
Owner Author

Choose a reason for hiding this comment

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

apsw uses them for figuring out how to create the virtual table accessor.

column_access: apsw.ext.VTColumnAccess
callable: Callable[[], Iterator[dict[str, Any]]]

def __call__(self) -> Iterator[dict[str, Any]]:
Copy link
Contributor

Choose a reason for hiding this comment

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

implementing __call__ is a little weird unless it's an API that apsw expects - and then, why not pass the generator where you need it?

Copy link
Owner Author

Choose a reason for hiding this comment

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

mypy or pyright (can't remember) were giving me typing issues.
The problem is that it just expects a generator but it wants to assign the properties column_access and columns on it.

Looks like the "way to do it" in python is to create a wrapper class basically I think.

Maybe I can use Protocols?

@fzakaria fzakaria merged commit fde5e41 into main Sep 22, 2023
2 checks passed
@fzakaria fzakaria deleted the simplify branch September 22, 2023 21:14
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.

None yet

2 participants