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

Warning fixes #144

Merged
merged 10 commits into from
Nov 20, 2023
Merged

Conversation

chrysn
Copy link
Collaborator

@chrysn chrysn commented Nov 20, 2023

Based on #142, this removes the #![allow(warnings)] (which AIU is a remnant of how early hacspec required writing code that the linter would be unhappy about), and applies a lot of small refactoring fixes that would probably just have been caught at write time / when they were earlier refactored into the current state.

The attribute only applies crate-wide.
Typical changes:
* No need to create a local early in the function, let alone populate it
  with a value when it will be populted with a value later anyway
* No need to carry the destructure output (worse, known-to-be-invalid
  because it's a field that was previously unused) around as mut;
  instead, it is assigned to an underscore variable (and dropped
  immediately because there is no Drop implementation; otherwise we'd
  have to assign to _ which is hard to read in tuple structs), and the
  local is created when it is computed.
Conflicts around edhoc.rs were resolved by going with the main version,
the fixes will be applied guided by `cargo check` later.
@chrysn
Copy link
Collaborator Author

chrysn commented Nov 20, 2023

Now ready to review (although best reviewed after #142); one warning is left (edhoc.rs line 432 about the kid never being assigned), but I suppose that's best addressed together with adding actual flexibility w/rt the peer's chosen credentials.

… explicit length

This fixes warnings that were legitimately raised about return values
ignored, because due to the different mechanisms of length transports,
the tests accessed different properties (the returned values) than the
implementation used (the old lengths).

This also implifies tests a lot, because there is way less copying
around between buffer types.
lib/src/edhoc.rs Outdated
@@ -45,7 +45,7 @@ pub fn edhoc_key_update(
_th_3,
) = state;

let mut prk_new_buf: BytesMaxBuffer = [0x00; MAX_BUFFER_LEN];
let mut prk_new_buf: BytesMaxBuffer;
Copy link
Collaborator

@geonnave geonnave Nov 20, 2023

Choose a reason for hiding this comment

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

This one we may have to revert, as hax does not like non-initialized variables. But it could be done in another moment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the preferred fix here is to not have a single buf but two bufs, not mutate them, and rely on the compiler to see that those two are not used at the same time and thus don't have to sit next to each other on the stack. Proposing the new fix (forcpushibly replacing the merge, which did both a merge and a fix at the same time)

The compiler can see that neither buffer implements a drop, they are not
used concurrently, and thus do not need separate space on the stack.
@geonnave geonnave merged commit c394fed into openwsn-berkeley:main Nov 20, 2023
28 checks passed
@chrysn chrysn deleted the warning-fixes branch November 20, 2023 16:03
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