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

Fix: Warning: calls to std::mem::drop #538

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yamakoud
Copy link
Contributor

@yamakoud yamakoud commented Mar 12, 2024

Issue: #523

What I did:
Fix warning calls to std::mem::drop

Why the problem occurred:
The std::mem::drop function is used to drop (dispose of) a value, taking ownership of that value as its argument.
In this case, a reference (&mut File) was passed to the function.
Since std::mem::drop targets values with ownership and not references, using it on a reference does nothing.
Therefore, this call to drop was unnecessary and the cause of the warning.

Test:

  • Build command succeed without any warnings
kyama@Nyanko-Book-Pro shiika % cd lib/skc_rustlib; cargo build; cd ../../
   Compiling libc v0.2.147
   Compiling regex-syntax v0.7.4
   Compiling ppv-lite86 v0.2.17
   Compiling uncased v0.9.9
   Compiling unicode-segmentation v1.10.1
   Compiling plain v0.2.3
   Compiling shiika_ffi_macro v0.1.0 (/Users/kyama/Projects/shiika/lib/shiika_ffi_macro)
   Compiling phf_shared v0.11.2
   Compiling phf_generator v0.11.2
   Compiling phf v0.11.2
   Compiling phf_codegen v0.11.2
   Compiling cc v1.0.82
   Compiling getrandom v0.2.10
   Compiling time v0.1.45
   Compiling rand_core v0.6.4
   Compiling rand_chacha v0.3.1
   Compiling chrono v0.4.26
   Compiling rand v0.8.5
   Compiling cmake v0.1.50
   Compiling regex-automata v0.3.6
   Compiling bdwgc-alloc v0.6.6
   Compiling regex v1.9.3
   Compiling parse-zoneinfo v0.3.0
   Compiling chrono-tz-build v0.0.3
   Compiling chrono-tz v0.6.3
   Compiling skc_rustlib v0.1.0 (/Users/kyama/Projects/shiika/lib/skc_rustlib)
    Finished dev [unoptimized + debuginfo] target(s) in 9.67s
    ```

@yamakoud yamakoud marked this pull request as ready for review March 12, 2024 15:48
@yamakoud
Copy link
Contributor Author

Ready for review

@yhara
Please take a look when you have time.

@yhara
Copy link
Collaborator

yhara commented Mar 13, 2024

Therefore, this call to drop was unnecessary and the cause of the warning.

Oh really? Let me explain what I wanted to do.

  • In Shiika, opened files should be closed when the File object is GC'ed (otherwise, the file is kept open until the end of the program)
  • However Rust does not provide explicit API like std::fs::close(). Instead, the file is closed when the fs::File is dropped
  • This std::mem::drop is intended to close the file manually

I thought once I confirmed that this closes the file, but forgot how... I'll try it later.

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