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.

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

chore: lots of lints and fixes #1523

Merged
merged 8 commits into from
Feb 7, 2024
Merged

chore: lots of lints and fixes #1523

merged 8 commits into from
Feb 7, 2024

Conversation

nyurik
Copy link
Contributor

@nyurik nyurik commented Feb 4, 2024 •
edited
Loading

  • it is an anti-pattern for a fn to take a reference, and clone it right away. If a fn needs to own a value, a value ownership should be passed in, not cloned inside the fn. This way the caller can decide to clone something, or to pass it in as is
  • use &Path instead of &PathBuf (same as &str instead of &String)
  • use .cmp for comparison
  • some spelling
  • lots of match statements can be simplified
  • use .is_null() instead of comparing with the null ptr
  • do not impl ToString trait, use Display instead, and get ToString for free - perf gain too.

@workingjubilee
Copy link
Member

there was a typo in pgrx/src/datum/datetime_support/ctor.rs - incorrectly declared conditional compilation for fn date_bin. Fixing it does not compile.

This is #1414

@workingjubilee
Copy link
Member

workingjubilee commented Feb 4, 2024 •
edited
Loading

No rustfmt?

Be aware this repo has a rustfmt.toml.

workingjubilee
workingjubilee reviewed Feb 4, 2024
View reviewed changes
pgrx/src/datum/date.rs Outdated
Comment on lines 154 to 155
let month: i32 = month.into();
let day: i32 = day.into();
Copy link
Member

Choose a reason for hiding this comment

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

These implementations existing are simply incorrect, do not use them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

would it make sense to perhaps implement into() as try_into().unwrap() ? In that case it would be consistent, and can be replaced.

workingjubilee
workingjubilee reviewed Feb 4, 2024
View reviewed changes
Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

Essentially none of the datetime types should actually implement From, even if they currently do, which is why so many dicey implementations and use of try_into() everywhere. I have to create more tests to verify the correctness of their implementations, and any change which removes these is breaking API, and the lifetimes problem has distracted me, which is also why I haven't shipped more of them:

pgrx/src/datum/array.rs Outdated Show resolved Hide resolved
pgrx/src/datum/array.rs Outdated Show resolved Hide resolved
pgrx/src/trigger_support/pg_trigger_when.rs Outdated Show resolved Hide resolved
pgrx/src/spi/tuple.rs Outdated Show resolved Hide resolved
pgrx/src/bgworkers.rs Outdated Show resolved Hide resolved
workingjubilee
workingjubilee reviewed Feb 4, 2024
View reviewed changes
pgrx/src/pg_sys.rs Outdated Show resolved Hide resolved
@nyurik
Copy link
Contributor Author

nyurik commented Feb 5, 2024

Thx for the feedback, addressed most of it in 582a0ac. Does it look better now?

@nyurik nyurik mentioned this pull request Feb 5, 2024
Merged
@nyurik nyurik force-pushed the lints branch 2 times, most recently from 8a8f670 to c185112 Compare February 5, 2024 23:05
@nyurik
Copy link
Contributor Author

nyurik commented Feb 5, 2024

No rustfmt?

Be aware this repo has a rustfmt.toml.

not sure what you mean - cargo fmt should pick up that file automatically. I did run cargo fmt on the whole repo. Also, just did a rebase to fix merge conflicts.

@workingjubilee
Copy link
Member

not sure what you mean - cargo fmt should pick up that file automatically.

I dunno, wasn't sure if it was simple omission or tooling or what.

@nyurik nyurik force-pushed the lints branch 4 times, most recently from 027ec3d to 366fb0b Compare February 7, 2024 03:22
workingjubilee
workingjubilee reviewed Feb 7, 2024
View reviewed changes
Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

Looks good, will doublecheck to see if it still looks good when I wake up.

pgrx/src/list/flat_list.rs Outdated Show resolved Hide resolved
@workingjubilee
Copy link
Member

Ah, this had conflicts with #1468, sorry!

@nyurik
Copy link
Contributor Author

nyurik commented Feb 7, 2024

rebasing...

nyurik and others added 8 commits February 7, 2024 15:59
@nyurik
chore: lots of lints and fixes
5ce3f85 
* it is an anti-pattern for a fn to take a reference, and clone it right away.  If a fn needs to own a value, a value ownership should be passed in, not cloned inside the fn.  This way the caller can decide to clone something, or to pass it in as is
* use `&Path` instead of `&PathBuf` (same as &str instead of &String)
* use `.cmp` for comparison
* some spelling
* remove some unneeded lifetimes
* lots of `try_into` can be replaced with `into`
* lots of `match` statements can be simplified
* use `.is_null()` instead of comparing with the null ptr
* do not impl `ToString` trait, use `Display` instead, and get ToString for free - perf gain too.

TODO:
* there was a typo in `pgrx/src/datum/datetime_support/ctor.rs` - incorrectly declared conditional compilation for `fn date_bin`.  Fixing it does not compile.
* `pgrx/src/spi/tuple.rs` has an incorrect double-usize casting, one should be removed, but it seems like it was a bug
@nyurik
a few more lints
3599ded
@nyurik
revert sum_vector_simd - might need benchmarks
1ece4aa
@nyurik
address feedback
8e40971
@nyurik
match improvements
8e70102
@nyurik
use suggested natts parsing
88f2e2d
@nyurik
use io::Error::Other
e26b8eb
@nyurik @workingjubilee
Update pgrx/src/list/flat_list.rs
4c31d5a 
Co-authored-by: Jubilee <[email protected]>
@nyurik
Copy link
Contributor Author

nyurik commented Feb 7, 2024

seems to be passing ок

workingjubilee
workingjubilee approved these changes Feb 7, 2024
View reviewed changes
Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

Yep, still looks good!

@workingjubilee workingjubilee merged commit 12842b6 into pgcentralfoundation:develop Feb 7, 2024
8 checks passed
@nyurik nyurik deleted the lints branch February 7, 2024 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@workingjubilee workingjubilee workingjubilee approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nyurik @workingjubilee