-
Notifications
You must be signed in to change notification settings - Fork 838
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
remove len field from StructBuilder #2468
Conversation
fn validate_content(&self) { | ||
if self.fields.len() != self.field_builders.len() { | ||
panic!("Number of fields is not equal to the number of field_builders."); | ||
} | ||
if !self.field_builders.iter().all(|x| x.len() == self.len) { | ||
if !self.field_builders.iter().all(|x| x.len() == self.len()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could help LLVM out and lift the len from the closure, it should be smart enough to do this automatically, but it can't hurt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the information, I wasn't aware of this.
I shall make this change tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Benchmark runs are scheduled for baseline = 63ab69e and contender = 42e9531. 42e9531 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #2429.
Rationale for this change
We can get the length of the StructBuilder from the null buffer builder.
So the field StructBuilder::len is useless
What changes are included in this PR?
Removed field len from StructBuilder
Are there any user-facing changes?
No