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

Refactor - FunctionRegistry #484

Merged
merged 2 commits into from
Jul 13, 2023
Merged

Refactor - FunctionRegistry #484

merged 2 commits into from
Jul 13, 2023

Conversation

Lichtso
Copy link

@Lichtso Lichtso commented Jul 11, 2023

Unifies the function registries of Executable and BuiltinProgram.

@Lichtso Lichtso force-pushed the refactor/function_registry branch 7 times, most recently from 0a5c3c2 to f6fa3a3 Compare July 11, 2023 16:00
@Lichtso Lichtso requested a review from alessandrod July 11, 2023 16:12
@alessandrod alessandrod requested review from alessandrod and removed request for alessandrod July 13, 2023 06:22
Copy link

@alessandrod alessandrod 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, see nits

src/elf.rs Outdated

impl<T: Copy + PartialEq> FunctionRegistry<T> {
/// Register a symbol with an explicit key
pub fn register_function(&mut self, key: u32, name: Vec<u8>, value: T) -> Result<(), ElfError> {

Choose a reason for hiding this comment

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

Here and in the other register_*, it should be name: impl Into<Vec<u8>>

src/elf.rs Outdated
} else {
let mut key = [0u8; mem::size_of::<u64>()];
LittleEndian::write_u64(&mut key, usize::from(value) as u64);
ebpf::hash_symbol_name(&key)

Choose a reason for hiding this comment

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

&usize::from(value).to_le_bytes() ?

@Lichtso Lichtso force-pushed the refactor/function_registry branch from f6fa3a3 to c795320 Compare July 13, 2023 11:14
@codecov-commenter
Copy link

Codecov Report

Merging #484 (c795320) into main (ff96604) will increase coverage by 0.15%.
The diff coverage is 81.59%.

@@            Coverage Diff             @@
##             main     #484      +/-   ##
==========================================
+ Coverage   89.51%   89.66%   +0.15%     
==========================================
  Files          23       23              
  Lines       10055    10095      +40     
==========================================
+ Hits         9001     9052      +51     
+ Misses       1054     1043      -11     
Impacted Files Coverage Δ
src/disassembler.rs 93.36% <50.00%> (ø)
src/vm.rs 73.47% <60.00%> (+3.27%) ⬆️
src/elf.rs 87.29% <80.15%> (+0.34%) ⬆️
src/static_analysis.rs 66.51% <91.66%> (+0.23%) ⬆️
src/assembler.rs 99.60% <100.00%> (-0.01%) ⬇️
src/interpreter.rs 97.92% <100.00%> (ø)
src/jit.rs 92.79% <100.00%> (+0.04%) ⬆️
src/verifier.rs 97.52% <100.00%> (ø)

@Lichtso Lichtso merged commit d5525c2 into main Jul 13, 2023
12 checks passed
@Lichtso Lichtso deleted the refactor/function_registry branch July 13, 2023 12:35
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