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

Fix (clippy) warnings in Rust generated code #230

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+2,868 −2,566
Open

Fix (clippy) warnings in Rust generated code #230

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0bef56e
Fix (clippy) warnings in Rust generate code
boxdot Aug 21, 2022
c15ecf2
Address review comments
boxdot Aug 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 5 additions & 7 deletions flatdata-generator/flatdata/generator/templates/rust/archive.jinja2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,15 +109,13 @@ impl ::std::fmt::Debug for {{archive.name}} {
}

impl {{archive.name}} {
#[allow(unused_imports, unused_variables)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a bit less granular, though, isn't it?

What's the problem with annotating exactly the imports/variable that might be unused?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It did not work in the first place:

683 |         #[allow(unused_variables)]
     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: if you just forgot a `!`, use: `#![allow(unused_var

There is no way to annotated a single import as maybe being unused.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But annotating a single variable is possible, should we do that?

pub fn open(storage: flatdata::StorageHandle)
-> ::std::result::Result<Self, flatdata::ResourceStorageError>
{
#[allow(unused_imports)]
use flatdata::SliceExt;
#[allow(unused_variables)]
use flatdata::ResourceStorageError as Error;
// extend lifetime since Rust cannot know that we reference a cache here
#[allow(unused_variables)]
let extend = |x : Result<&[u8], Error>| -> Result<&'static [u8], Error> {x.map(|x| unsafe{std::mem::transmute(x)})};

storage.read(&Self::signature_name("{{archive.name}}"), schema::{{ archive_ns }}::{{ archive.name | camel_to_snake_case | upper }})?;
Expand All @@ -129,17 +127,17 @@ impl {{archive.name}} {
{% if [r] | vector_resources %}
{% set t = fully_qualified_name(archive, r.referenced_structures[0].node) %}
let resource = extend(storage.read("{{r.name}}", schema::{{ archive_ns }}::resources::{{ r.name | upper }}));
check("{{ r.name }}", |r| r.len(), max_size, resource.and_then(|x| <&[{{t}}]>::from_bytes(x)))?
check("{{ r.name }}", |r| r.len(), max_size, resource.and_then(<&[{{t}}]>::from_bytes))?
{% elif [r] | instance_resources %}
{% set t = fully_qualified_name(archive, r.referenced_structures[0].node) %}
let resource = extend(storage.read("{{r.name}}", schema::{{ archive_ns }}::resources::{{ r.name | upper }}));
check("{{ r.name }}", |_| 0, max_size, resource.and_then(|x| {{t}}::from_bytes_slice(x)))?
check("{{ r.name }}", |_| 0, max_size, resource.and_then({{t}}::from_bytes_slice))?
{% elif [r] | rawdata_resources %}
let resource = extend(storage.read("{{r.name}}", schema::{{ archive_ns }}::resources::{{ r.name | upper }}));
check("{{ r.name }}", |r| r.len(), max_size, resource.map(|x| flatdata::RawData::new(x)))?
check("{{ r.name }}", |r| r.len(), max_size, resource.map(flatdata::RawData::new))?
{% elif [r] | multivector_resources %}
{% set i = fully_qualified_name(archive, r.index_reference.node) %}
let index_schema = &format!("index({})", schema::{{ archive_ns }}::resources::{{ r.name | upper }});
let index_schema = format!("index({})", schema::{{ archive_ns }}::resources::{{ r.name | upper }});
let index = extend(storage.read("{{r.name}}_index", &index_schema));
let data = extend(storage.read("{{r.name}}", schema::{{ archive_ns }}::resources::{{ r.name | upper }}));
let result = match (index, data) {
Expand Down
6 changes: 6 additions & 0 deletions flatdata-generator/flatdata/generator/templates/rust/structure.jinja2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ pub struct {{struct.name}} {
}

impl {{ struct.name }} {
/// # Safety
///
/// Unsafe since the struct might not be self-contained
pub unsafe fn new_unchecked( ) -> Self {
Self{data : [0; {{ struct.size_in_bytes }}]}
Expand Down Expand Up @@ -45,6 +47,7 @@ impl {{ struct.name }} {
}

/// Create reference from byte array
#[allow(clippy::len_zero)]
pub fn from_bytes_slice(data: &[u8]) -> Result<&Self, flatdata::ResourceStorageError> {
// We cannot rely on TryFrom here, since it does not yet support > 33 bytes
if data.len() < {{ struct.size_in_bytes }} {
Expand All @@ -57,6 +60,7 @@ impl {{ struct.name }} {
}

/// Create reference from byte array
#[allow(clippy::len_zero)]
pub fn from_bytes_slice_mut(data: &mut [u8]) -> Result<&mut Self, flatdata::ResourceStorageError> {
// We cannot rely on TryFrom here, since it does not yet support > 33 bytes
if data.len() < {{ struct.size_in_bytes }} {
Expand Down Expand Up @@ -94,6 +98,7 @@ impl {{ struct.name }} {
/// [`{{ field.range | escape_rust_keywords }}`]: #method.{{ field.range | escape_rust_keywords }}
{% endif %}
#[inline]
#[allow(clippy::useless_transmute)]
pub fn {{ field.name | escape_rust_keywords }}(&self) -> {% if field.invalid_value %}Option<{{ field | field_type }}>{% else %}{{ field | field_type }}{% endif %} {
let value = flatdata_read_bytes!({{ field | primitive_type }}, self.data.as_ptr(), {{ field.offset }}, {{ field.type.width }});
{% if field.invalid_value %}
Expand All @@ -109,6 +114,7 @@ impl {{ struct.name }} {
{{ field.doc | rust_doc }}
{% endif %}
#[inline]
#[allow(clippy::identity_op)]
pub fn {{ field.range | escape_rust_keywords }}(&self) -> std::ops::Range<{% if field.invalid_value %}Option<{{ field | field_type }}>{% else %}{{ field | field_type }}{% endif %}> {
let start = flatdata_read_bytes!({{ field.type.name }}, self.data.as_ptr(), {{ field.offset }}, {{ field.type.width }});
let end = flatdata_read_bytes!({{ field.type.name }}, self.data.as_ptr(), {{ field.offset }} + {{ struct.size_in_bytes }} * 8, {{ field.type.width }});
Expand Down
4 changes: 2 additions & 2 deletions flatdata-generator/flatdata/generator/templates/rust/variadic.jinja2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ impl<'a> {{name}}Builder<'a> {
///
/// [`{{type.node.name}}`]: struct.{{type.node.name}}.html
#[inline]
pub fn add_{{ type.node.name | camel_to_snake_case }}<'b>(&'b mut self) -> &'b mut {{inner_type}} {
pub fn add_{{ type.node.name | camel_to_snake_case }}(&mut self) -> &mut {{inner_type}} {
let old_len = self.data.len();
let increment = 1 + <{{inner_type}} as flatdata::Struct>::SIZE_IN_BYTES;
self.data.resize(old_len + increment, 0);
Expand Down Expand Up @@ -101,7 +101,7 @@ impl<'a> flatdata::VariadicStruct<'a> for {{name}} {
match index {
{% for type in types %}
{% set inner_type = fully_qualified_name(archive, type.node) %}
{{loop.index0}} => {{name}}Ref::{{type.node.name}}({{inner_type}}::from_bytes_slice(&data).expect("Corrupted data")),
{{loop.index0}} => {{name}}Ref::{{type.node.name}}({{inner_type}}::from_bytes_slice(data).expect("Corrupted data")),
{% endfor %}
_ => panic!("invalid type index {} for variadic type {{name}}Ref", index),
}
Expand Down
14 changes: 4 additions & 10 deletions flatdata-generator/tests/generators/rust_expectations/archives/comments.rs.1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,13 @@ impl ::std::fmt::Debug for Foo {
}

impl Foo {
#[allow(unused_imports, unused_variables)]
pub fn open(storage: flatdata::StorageHandle)
-> ::std::result::Result<Self, flatdata::ResourceStorageError>
{
#[allow(unused_imports)]
use flatdata::SliceExt;
#[allow(unused_variables)]
use flatdata::ResourceStorageError as Error;
// extend lifetime since Rust cannot know that we reference a cache here
#[allow(unused_variables)]
let extend = |x : Result<&[u8], Error>| -> Result<&'static [u8], Error> {x.map(|x| unsafe{std::mem::transmute(x)})};

storage.read(&Self::signature_name("Foo"), schema::foo::FOO)?;
Expand All @@ -44,10 +42,9 @@ impl Foo {
use flatdata::check_resource as check;
let max_size = None;
let resource = extend(storage.read("bar", schema::foo::resources::BAR));
check("bar", |r| r.len(), max_size, resource.map(|x| flatdata::RawData::new(x)))?
check("bar", |r| r.len(), max_size, resource.map(flatdata::RawData::new))?
};


Ok(Self {
_storage: storage,
bar,
Expand Down Expand Up @@ -115,15 +112,13 @@ impl ::std::fmt::Debug for Bar {
}

impl Bar {
#[allow(unused_imports, unused_variables)]
pub fn open(storage: flatdata::StorageHandle)
-> ::std::result::Result<Self, flatdata::ResourceStorageError>
{
#[allow(unused_imports)]
use flatdata::SliceExt;
#[allow(unused_variables)]
use flatdata::ResourceStorageError as Error;
// extend lifetime since Rust cannot know that we reference a cache here
#[allow(unused_variables)]
let extend = |x : Result<&[u8], Error>| -> Result<&'static [u8], Error> {x.map(|x| unsafe{std::mem::transmute(x)})};

storage.read(&Self::signature_name("Bar"), schema::bar::BAR)?;
Expand All @@ -132,10 +127,9 @@ impl Bar {
use flatdata::check_resource as check;
let max_size = None;
let resource = extend(storage.read("foo", schema::bar::resources::FOO));
check("foo", |r| r.len(), max_size, resource.map(|x| flatdata::RawData::new(x)))?
check("foo", |r| r.len(), max_size, resource.map(flatdata::RawData::new))?
};


Ok(Self {
_storage: storage,
foo,
Expand Down
6 changes: 2 additions & 4 deletions flatdata-generator/tests/generators/rust_expectations/archives/empty.rs.1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,13 @@ impl ::std::fmt::Debug for A {
}

impl A {
#[allow(unused_imports, unused_variables)]
pub fn open(storage: flatdata::StorageHandle)
-> ::std::result::Result<Self, flatdata::ResourceStorageError>
{
#[allow(unused_imports)]
use flatdata::SliceExt;
#[allow(unused_variables)]
use flatdata::ResourceStorageError as Error;
// extend lifetime since Rust cannot know that we reference a cache here
#[allow(unused_variables)]
let extend = |x : Result<&[u8], Error>| -> Result<&'static [u8], Error> {x.map(|x| unsafe{std::mem::transmute(x)})};

storage.read(&Self::signature_name("A"), schema::a::A)?;
Expand Down Expand Up @@ -54,4 +52,4 @@ impl ABuilder {
flatdata::create_archive("A", schema::a::A, &storage)?;
Ok(Self { storage })
}
}
}
Loading