Add `#[derive(Clone, Copy...` on all bitflags by nyurik · Pull Request #1396 · rusqlite/rusqlite

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

 /// 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

 // 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


 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

 #[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

 /// 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

 // ffi::sqlite3_vtab_cursor => VTabCursor

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


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

src/session.rs

 #[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


 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

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

Conversation

nyurik commented Oct 6, 2023

Uh oh!

codecov bot commented Oct 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

gwenn commented Oct 7, 2023

Uh oh!

nyurik commented Oct 7, 2023

Uh oh!

gwenn commented Oct 7, 2023

Uh oh!

nyurik commented Oct 7, 2023

Uh oh!

gwenn commented Oct 7, 2023

Uh oh!

nyurik commented Oct 7, 2023

Uh oh!

nyurik commented Dec 17, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nyurik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Oct 6, 2023 •

edited

Loading