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

Data loading #16

Closed
wants to merge 2 commits into from
Closed

Data loading #16

wants to merge 2 commits into from

Conversation

ksew1
Copy link
Member

@ksew1 ksew1 commented Aug 8, 2024

Closes #9

@ksew1 ksew1 force-pushed the data-loading branch 2 times, most recently from 77dca6b to 5afee18 Compare August 8, 2024 14:51
crates/cairo-coverage/src/inner/config.rs Outdated Show resolved Hide resolved
crates/cairo-coverage/src/inner/config.rs Outdated Show resolved Hide resolved
crates/cairo-coverage/src/inner/config.rs Outdated Show resolved Hide resolved
crates/cairo-coverage/src/inner/data_loader/types.rs Outdated Show resolved Hide resolved
const PROFILER_NAMESPACE: &str = "github.com/software-mansion/cairo-profiler";
const COVERAGE_NAMESPACE: &str = "github.com/software-mansion/cairo-coverage";

type FileLocation = String;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be a path.

Copy link
Member Author

Choose a reason for hiding this comment

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

But we also have items like this:

/Users/makysmiliandemitraszek/Library/Caches/com.swmansion.scarb/registry/std/e512d6dd1/core/src/lib.cairo[array_inline_macro]

which is not really a path.

crates/cairo-coverage/src/inner/data_loader/types.rs Outdated Show resolved Hide resolved
}

#[derive(Deserialize, Clone, Debug, Eq, PartialEq)]
pub struct Range {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not informative name.

crates/cairo-coverage/src/inner/data_loader/types.rs Outdated Show resolved Hide resolved
Comment on lines 78 to 79
// Safe to unwrap because we ensured that the keys are the same
let function_names = statements_functions.get(&key).unwrap().to_owned();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is better to do it in a safe way. You don't have to hold the pre-conditions in your head this way and you don't have to trust your preconditions are correct. This is a very good feature of the language, there is no point of throwing it away unless necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this could be written succinctly as
collect should handle this for you

 let statement_map: Result<Map<...>> = statements_code_locations
            .into_iter()
            .map(|(key, code_locations)| {
                // Safe to unwrap because we ensured that the keys are the same
                let Some(function_names) = statements_functions.get(&key).unwrap().to_owned();
                Ok((
                    key,
                    StatementDetails {
                        code_locations,
                        function_names,
                    },
                ))
            } else {
                Error("msg")
            }
            )
            .collect();

#[allow(dead_code)] // Temporary
pub fn new(
call_trace_path: &Utf8PathBuf,
versioned_program_path: &Utf8PathBuf,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is versioned_program_path a parameter here? Shouldn't we rather take this information from call_trace -> cairo_execution_info -> source_sierra_path?

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that this is the same file as test_project_Tests.sierra.json. If that's the case can we avoid duplicating this file?

Cargo.toml Outdated Show resolved Hide resolved
Data loading
@ksew1 ksew1 closed this Aug 25, 2024
@ksew1 ksew1 deleted the data-loading branch September 4, 2024 09:59
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.

Data loading
3 participants