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

add size parameter for vec_box lint #5081

Merged
merged 1 commit into from
Jan 25, 2020

Conversation

basil-cow
Copy link
Contributor

changelog: add size threshold for the vec_box lint, currently 4096 bytes (one page) (subject to change). Closes #3547.

diff is a little bit confusing due to some refactoring (moving free functions to lint struct functions), relevant portion is this. In hindsight should've been different commits, but oh well.

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

Impl looks good, test is missing.

@basil-cow basil-cow force-pushed the vec_box_threshold branch 3 times, most recently from 9f858e1 to 90697bf Compare January 23, 2020 18:53
@@ -152,6 +152,8 @@ define_Conf! {
(too_many_lines_threshold, "too_many_lines_threshold", 100 => u64),
/// Lint: LARGE_STACK_ARRAYS. The maximum allowed size for arrays on the stack
(array_size_threshold, "array_size_threshold", 512_000 => u64),
/// Lint: VEC_BOX. The minimum required size for boxed type in Vec
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Lint: VEC_BOX. The minimum required size for boxed type in Vec
/// Lint: VEC_BOX. The size of the boxed type in bytes, where boxing in a `Vec` is allowed

Is 4kB based on something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Page size; but as I said in the pr, you are welcome to put any number you like

Copy link
Member

Choose a reason for hiding this comment

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

Page size sounds good.

@flip1995
Copy link
Member

@bors r+

Thanks!

@bors
Copy link
Collaborator

bors commented Jan 25, 2020

📌 Commit 2b7bc26 has been approved by flip1995

bors added a commit that referenced this pull request Jan 25, 2020
add size parameter for `vec_box`  lint

changelog: add size threshold for the `vec_box` lint, currently 4096 bytes (one page) (subject to change). Closes #3547.

diff is a little bit confusing due to some refactoring (moving free functions to lint struct functions), relevant portion is [this](https://github.com/rust-lang/rust-clippy/compare/master...Areredify:vec_box_threshold?expand=1#diff-1096120ca9143af89dcc9175ea92b54aR294-R298). In hindsight should've been different commits, but oh well.
@bors
Copy link
Collaborator

bors commented Jan 25, 2020

⌛ Testing commit 2b7bc26 with merge 6b54194...

@bors
Copy link
Collaborator

bors commented Jan 25, 2020

☀️ Test successful - checks-travis, status-appveyor
Approved by: flip1995
Pushing 6b54194 to master...

@bors bors merged commit 2b7bc26 into rust-lang:master Jan 25, 2020
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.

Make VEC_BOX lint configurable over the size of T
3 participants