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

Add clippy and fmt #5

Merged
merged 3 commits into from
Mar 5, 2024
Merged

Add clippy and fmt #5

merged 3 commits into from
Mar 5, 2024

Conversation

gowthamsk-arm
Copy link
Collaborator

Adds clippy and fmt to the CI along with changes needed to pass.

Signed-off-by: Gowtham Suresh Kumar [email protected]

Signed-off-by: Gowtham Suresh Kumar <[email protected]>
Signed-off-by: Gowtham Suresh Kumar <[email protected]>
Copy link
Member

@tgonzalezorlandoarm tgonzalezorlandoarm left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -1,5 +1,7 @@
// Copyright 2023 Contributors to the Parsec project.
// SPDX-License-Identifier: Apache-2.0
#![allow(clippy::missing_safety_doc)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is clippy using publicly visible unsafe functions ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes the parsec_provider_provider_init here is dealing with C pointers as it will be at the FFI boundary.

ci.sh Outdated
shift
done

if [ "$BUILD_AND_TEST" ]; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is tricky. It will work as is, but it can be prone to bugs.

We should either check that the variable is non zero len [[ -n $var ]] or perform a string comparison.

Or we can use boolean variables using BUILD_AND_TEST=false at line 32 and BUILD_AND_TEST=true

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry for the delay. Yes, I will update it with a boolean.

Copy link
Collaborator

@minosgalanakis minosgalanakis left a comment

Choose a reason for hiding this comment

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

Good overall, two tibits only .

This patch adds a ci_script GitHub action and splits the jobs to
run specific tests.

Signed-off-by: Gowtham Suresh Kumar <[email protected]>
Copy link
Collaborator

@minosgalanakis minosgalanakis left a comment

Choose a reason for hiding this comment

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

LGTM

@gowthamsk-arm gowthamsk-arm merged commit 6312150 into main Mar 5, 2024
7 checks passed
@gowthamsk-arm gowthamsk-arm deleted the ci_test branch March 11, 2024 16:17
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