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.

Sign up for GitHub

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

Jump to bottom

Clippy Lints #37

Open

cholcombe973 wants to merge 3 commits into master from clippy

Contributor

cholcombe973 commented Jan 9, 2019 •

edited

Loading

Warning! Many breaking changes here from clippy suggestions about best practices.


          Clippy lints

2361d72

cholcombe973 requested a review from ChrisMacNaughton

January 9, 2019 00:35

ChrisMacNaughton requested changes

View reviewed changes

Collaborator

ChrisMacNaughton left a comment

While I'm generally happy with passing pointers instead of moving values, I'd like to understand the reasoning for these type changes, as well as the additional changes of pointer -> move

src/ceph.rs

@@ @@ -1099,7 +1110,7 @@ impl IoCtx { @@
                   }
                   /// Update tmap (trivial map)
-                  pub fn rados_object_tmap_update(&self, object_name: &str, update: TmapOperation) -> RadosResult<()> {
+                  pub fn rados_object_tmap_update(&self, object_name: &str, update: &TmapOperation) -> RadosResult<()> {

Collaborator

ChrisMacNaughton Jan 9, 2019

Does this need to change from pass by value to a pointer?

src/ceph.rs

@@ @@ -1292,7 +1303,7 @@ impl IoCtx { @@
                   }
                   // Perform a compound read operation synchronously
-                  pub fn rados_perform_read_operations(&self, read_op: ReadOperation) -> RadosResult<()> {
+                  pub fn rados_perform_read_operations(&self, read_op: &ReadOperation) -> RadosResult<()> {

Collaborator

ChrisMacNaughton Jan 9, 2019

Does this need to change from pass by value to a pointer?

src/ceph.rs

    
            @@ -1937,7 +1939,7 @@ impl Rados {
          
                      name: &str,

                      value: &str,

                      format: Option<&str>,

                      data: Vec<*mut c_char>,

                      data: &[*mut c_char],

Collaborator

ChrisMacNaughton Jan 9, 2019

Does this need to change from pass by value to a pointer?

src/ceph.rs

    
            @@ -2025,7 +2027,7 @@ impl Rados {
          
                      name: &str,

                      value: &str,

                      format: Option<&str>,

                      data: Vec<*mut c_char>,

                      data: &[*mut c_char],

Collaborator

ChrisMacNaughton Jan 9, 2019

Does this need to change from pass by value to a pointer?

src/ceph.rs

    
            @@ -2112,7 +2114,7 @@ impl Rados {
          
                      name: &str,

                      value: &str,

                      format: Option<&str>,

                      data: Vec<*mut c_char>,

                      data: &[*mut c_char],

Collaborator

ChrisMacNaughton Jan 9, 2019

Does this need to change from pass by value to a pointer?

src/ceph_client.rs

@@ @@ -279,7 +287,7 @@ impl CephClient { @@
                       Ok(cmd::mgr_versions(&self.rados_t)?)
                   }
-                  pub fn run_command(&self, command: MonCommand) -> Result<String, RadosError> {
+                  pub fn run_command(&self, command: &MonCommand) -> Result<String, RadosError> {

Collaborator

ChrisMacNaughton Jan 9, 2019

Does this need to change from pass by value to a pointer?

src/cmd.rs

               }
               /// Query a ceph pool.
-              pub fn osd_pool_get(cluster_handle: &Rados, pool: &str, choice: &PoolOption) -> RadosResult<String> {
+              pub fn osd_pool_get(cluster_handle: &Rados, pool: &str, choice: PoolOption) -> RadosResult<String> {

Collaborator

ChrisMacNaughton Jan 9, 2019

Unlike the (many) comments about moves vs pointers, the ones in this file are the reverse?

cholcombe973 added 2 commits

January 9, 2019 09:39


          Fix unsafe pointers

1fc6eac


          Rustfmt and use ? operator

727bf4e

Also fix some double pointers and use rados_buffer_free for
ceph heap allocated memory. Prevent mem leaks

Contributor Author

cholcombe973 commented Jan 9, 2019

Yeah all of these changes are clippy suggestions that values weren't consumed by a function so they should be passed by reference or that enum's passed by reference would be more efficient if they were copied. I don't know how true the copy efficiency is because i haven't benchmarked it.

Xuanwo force-pushed the master branch 2 times, most recently from ead7751 to 9b39b74 Compare

September 3, 2024 13:14

Collaborator

Xuanwo commented Sep 3, 2024

Hi, @cholcombe973, would you be interested in working on this again?

Contributor Author

cholcombe973 commented Sep 3, 2024 via email

Yeah I could try a rebase and clippy again to see what happens.

On Tue, Sep 3, 2024 at 7:23 AM Xuanwo ***@***.***> wrote: Hi, @cholcombe973 <https://github.com/cholcombe973>, would you be interested in working on this again? — Reply to this email directly, view it on GitHub <#37 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAXKUE23AB4IUCB7HTSCEWLZUXA73AVCNFSM6AAAAABNSILQTKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMRWGY3DEMJYG4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Collaborator

Xuanwo commented Sep 3, 2024 •

edited

Loading

Yeah I could try a rebase and clippy again to see what happens.

Perfect, thank you so much! I'm so happy you're still here, in this community.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet