Add `#[derive(Clone, Copy...` on all bitflags #1396

nyurik · 2023-10-06T23:44:08Z

This will make it easier to use them, e.g. I won't need to re-create FunctionFlags for registering multiple similar functions.

codecov · 2023-10-06T23:50:41Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (fc738f3) 81.08% compared to head (4e0b446) 81.08%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1396   +/-   ##
=======================================
  Coverage   81.08%   81.08%           
=======================================
  Files          51       51           
  Lines       10345    10345           
=======================================
  Hits         8388     8388           
  Misses       1957     1957

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

gwenn · 2023-10-07T07:28:21Z

Eq and PartialEq seem useless

nyurik · 2023-10-07T07:41:17Z

They are clearly not as useful as Copy, agree, but I am sure there could be usecases where users will want to pass them around and compare them, wouldn't they? Also, there could be some implementers that wrap your library, and they might need to take extra actions before passing bitfield into the inner lib. But this is up to you - my main req is to copy bitfields to pass them around more easily (e.g. have a global const/static, etc).

Speaking of derive - there are many enums that also do not implement Copy, e.g. types::Type should be both Copy and Hash (someone may want to have a lookup for type -> value conversion of sorts?). Do you have any overall rule of thumb of what gets which derives? Thx!

gwenn · 2023-10-07T07:45:15Z

I am sure there could be usecases where ...

YAGNI: https://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it

nyurik · 2023-10-07T07:51:24Z

@gwenn YAGNII is my fav principle too, but the cost of submitting a PR and discussing it, and waiting for release is also pretty high, so what's the middle ground? I will add the clone/copy where makes sense - that's what I do need. Is that ok?

gwenn · 2023-10-07T07:58:24Z

I will add the clone/copy where makes sense - that's what I do need. Is that ok?

Yes, please.

nyurik · 2023-10-07T08:15:07Z

i tried to clean them up a bit, adding just the copy/clone where it seem to make sense. Overall, looking at many pub enum cases, it seems the derives are very unorganized/ad-hoc, without much consistency. I will try to tame my OCD though :)

This will make it easier to use them, e.g. I won't need to re-create `FunctionFlags` for registering multiple similar functions.

nyurik · 2023-12-17T18:53:02Z

@gwenn should i make any changes to this PR?

gwenn · 2023-12-23T12:16:33Z

src/vtab/mod.rs

@@ -322,7 +323,7 @@ pub trait UpdateVTab<'vtab>: CreateVTab<'vtab> {

 /// Index constraint operator.
 /// See [Virtual Table Constraint Operator Codes](https://sqlite.org/c3ref/c_index_constraint_eq.html) for details.
-#[derive(Debug, Eq, PartialEq)]
+#[derive(Copy, Clone, Debug, Eq, PartialEq)]


Created only by SQLite itself (readonly on rusqlite side)

gwenn · 2023-12-23T12:17:56Z

src/vtab/mod.rs

@@ -59,6 +59,7 @@ use crate::{str_to_cstring, Connection, Error, InnerConnection, Result};
 // ffi::sqlite3_vtab_cursor => VTabCursor

 /// Virtual table kind
+#[derive(Copy, Clone, Debug)]


Used only as a constant at compile time.

gwenn · 2023-12-23T12:23:14Z

src/vtab/series.rs

@@ -28,7 +28,7 @@ const SERIES_COLUMN_STOP: c_int = 2;
 const SERIES_COLUMN_STEP: c_int = 3;

 bitflags::bitflags! {
-    #[derive(Clone, Copy)]
+    #[derive(Clone, Copy, Debug)]


Use only to pass information between best_index and filter methods.

gwenn · 2023-12-23T12:29:04Z

src/session.rs

@@ -655,7 +655,7 @@ impl Connection {
 /// See [here](https://sqlite.org/session.html#SQLITE_CHANGESET_CONFLICT) for details.
 #[allow(missing_docs)]
 #[repr(i32)]
-#[derive(Debug, PartialEq, Eq)]
+#[derive(Copy, Clone, Debug, PartialEq, Eq)]


Created only by SQLite (readonly on rusqlite side)

nyurik

thx, reverted the ones you suggested, and also merged main for ease of review. You may want to squash when merging though

nyurik · 2023-12-24T04:16:20Z

src/vtab/mod.rs

@@ -322,7 +323,7 @@ pub trait UpdateVTab<'vtab>: CreateVTab<'vtab> {

 /// Index constraint operator.
 /// See [Virtual Table Constraint Operator Codes](https://sqlite.org/c3ref/c_index_constraint_eq.html) for details.
-#[derive(Debug, Eq, PartialEq)]
+#[derive(Copy, Clone, Debug, Eq, PartialEq)]


nyurik · 2023-12-24T04:16:23Z

src/vtab/mod.rs

@@ -59,6 +59,7 @@ use crate::{str_to_cstring, Connection, Error, InnerConnection, Result};
 // ffi::sqlite3_vtab_cursor => VTabCursor

 /// Virtual table kind
+#[derive(Copy, Clone, Debug)]


nyurik · 2023-12-24T04:16:25Z

src/session.rs

@@ -655,7 +655,7 @@ impl Connection {
 /// See [here](https://sqlite.org/session.html#SQLITE_CHANGESET_CONFLICT) for details.
 #[allow(missing_docs)]
 #[repr(i32)]
-#[derive(Debug, PartialEq, Eq)]
+#[derive(Copy, Clone, Debug, PartialEq, Eq)]


nyurik · 2023-12-24T04:16:36Z

src/vtab/series.rs

@@ -28,7 +28,7 @@ const SERIES_COLUMN_STOP: c_int = 2;
 const SERIES_COLUMN_STEP: c_int = 3;

 bitflags::bitflags! {
-    #[derive(Clone, Copy)]
+    #[derive(Clone, Copy, Debug)]


nyurik force-pushed the impl-bit-traits branch from 097ce05 to 79d8edd Compare October 7, 2023 08:13

Add #[derive(Clone, Copy... on all bitflags

7c58506

This will make it easier to use them, e.g. I won't need to re-create `FunctionFlags` for registering multiple similar functions.

nyurik force-pushed the impl-bit-traits branch from 79d8edd to 7c58506 Compare October 19, 2023 22:14

gwenn reviewed Dec 23, 2023

View reviewed changes

nyurik added 2 commits December 23, 2023 23:13

Merge branch 'main' into impl-bit-traits

be4f1de

review feedback

4e0b446

nyurik commented Dec 24, 2023

View reviewed changes

gwenn merged commit 68c3083 into rusqlite:master Dec 24, 2023
16 checks passed

nyurik deleted the impl-bit-traits branch December 24, 2023 08:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `#[derive(Clone, Copy...` on all bitflags #1396

Add `#[derive(Clone, Copy...` on all bitflags #1396

nyurik commented Oct 6, 2023

codecov bot commented Oct 6, 2023 •

edited

gwenn commented Oct 7, 2023

nyurik commented Oct 7, 2023

gwenn commented Oct 7, 2023

nyurik commented Oct 7, 2023

gwenn commented Oct 7, 2023

nyurik commented Oct 7, 2023

nyurik commented Dec 17, 2023

gwenn Dec 23, 2023

nyurik Dec 24, 2023

gwenn Dec 23, 2023

nyurik Dec 24, 2023

gwenn Dec 23, 2023

nyurik Dec 24, 2023

gwenn Dec 23, 2023

nyurik Dec 24, 2023

nyurik left a comment

nyurik Dec 24, 2023

nyurik Dec 24, 2023

nyurik Dec 24, 2023

nyurik Dec 24, 2023

Add #[derive(Clone, Copy... on all bitflags #1396

Add #[derive(Clone, Copy... on all bitflags #1396

Conversation

nyurik commented Oct 6, 2023

codecov bot commented Oct 6, 2023 • edited

Codecov Report

gwenn commented Oct 7, 2023

nyurik commented Oct 7, 2023

gwenn commented Oct 7, 2023

nyurik commented Oct 7, 2023

gwenn commented Oct 7, 2023

nyurik commented Oct 7, 2023

nyurik commented Dec 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nyurik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `#[derive(Clone, Copy...` on all bitflags #1396

Add `#[derive(Clone, Copy...` on all bitflags #1396

codecov bot commented Oct 6, 2023 •

edited