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

Move Preprocessed Columns Struct to Preprocessed File #359

Merged
merged 1 commit into from
Jan 16, 2025

Conversation

Gali-StarkWare
Copy link
Contributor

@Gali-StarkWare Gali-StarkWare commented Jan 16, 2025

This change is Reviewable

Copy link
Contributor Author

Gali-StarkWare commented Jan 16, 2025

@Gali-StarkWare Gali-StarkWare marked this pull request as ready for review January 16, 2025 13:01
@Gali-StarkWare Gali-StarkWare self-assigned this Jan 16, 2025
Copy link
Contributor

@shaharsamocha7 shaharsamocha7 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Gali-StarkWare)

pub fn packed_at(&self, vec_row: usize) -> PackedM31 {
assert!(vec_row < (1 << self.log_size) / N_LANES);
PackedM31::broadcast(M31::from(vec_row * N_LANES))
+ unsafe { PackedM31::from_simd_unchecked(SIMD_ENUMERATION_0) }

Choose a reason for hiding this comment

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

Semgrep identified an issue in your code:

Detected 'unsafe' usage, please audit for secure usage

To resolve this comment:

✨ Commit Assistant fix suggestion

Suggested change
+ unsafe { PackedM31::from_simd_unchecked(SIMD_ENUMERATION_0) }
// Using `from_simd_unchecked` is necessary here because we are directly converting
// a SIMD enumeration to a PackedM31 type. Ensure that `SIMD_ENUMERATION_0` is valid
// and correctly aligned for this operation. If the library provides a safe alternative,
// consider using it instead.
+ unsafe { PackedM31::from_simd_unchecked(SIMD_ENUMERATION_0) }
View step-by-step instructions
  1. Review the usage of unsafe in PackedM31::from_simd_unchecked(SIMD_ENUMERATION_0) to understand why it is necessary. Ensure that the operation is safe and does not lead to undefined behavior.
  2. If the unsafe block is necessary, document why it is safe in this context. Add a comment above the unsafe block explaining the reasoning and any assumptions made.
  3. If possible, replace the unsafe block with a safe alternative. Check if there is a safe API provided by the library that can be used instead of from_simd_unchecked.
  4. If no safe alternative exists and the unsafe block is unavoidable, ensure that all inputs and operations within the block are validated to prevent any potential undefined behavior.
💬 Ignore this finding

Reply with Semgrep commands to ignore this finding.

  • /fp <comment> for false positive
  • /ar <comment> for acceptable risk
  • /other <comment> for all other reasons

Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by unsafe-usage.

You can view more details about this finding in the Semgrep AppSec Platform.

Copy link
Contributor Author

@Gali-StarkWare Gali-StarkWare left a comment

Choose a reason for hiding this comment

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

Dismissed @semgrep-code-starkware-libs[bot] from a discussion.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @shaharsamocha7)

@Gali-StarkWare Gali-StarkWare merged commit e9f0546 into main Jan 16, 2025
17 checks passed
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.

2 participants