Skip to content

Add a function to produce merged peripheral when derivedFrom is used. #49

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

Merged
merged 1 commit into from
Mar 17, 2018

Conversation

nsabovic
Copy link
Contributor

Added a function to make a new peripheral by overlaying the peripheral over its derivedFrom peripheral. I read the spec that it is a shallow copy, but I could be wrong. This may be part of #21.

With this change, and another one that I have for svd2rust, svd2rust can generate peripherals with derivedFrom. I could have put this function over there, but I think the way overlaying is done is a part of the SVD spec, and not part of the particular svd2rust implementation, so it makes more sense for me to put it here.

@Emilgardis
Copy link
Member

I'm not sure about having this in this crate. What reasons are there for it being in this crate instead of having it as a function in svd2rust?

@nsabovic
Copy link
Contributor Author

nsabovic commented Mar 11, 2018

Could just as well be there, if this crate is just to be tasked with parsing. However, if someone wanted to write svd2c++, they would also need this function. It is only called once in svd2rust, it would be easy to stick it there, if that were your design preference.

@@ -178,6 +178,23 @@ pub struct Peripheral {
}

impl Peripheral {
pub fn derive_from(&self, other: &Peripheral) -> Peripheral {
let mut derived = self.clone();
if derived.group_name.is_none() && other.group_name.is_some() {
Copy link
Contributor

Choose a reason for hiding this comment

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

minor nit: the conditionals here don't need to check other.XXX. If they're is_none then the derived value will get assigned to None anyway. It just feels a little less repetitive:

  if derived.group_name.is_none() {
       derived.group_name = other.group_name.clone();
  }

Copy link
Member

Choose a reason for hiding this comment

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

I second this. I'll merge this when this has been fixed.

@wez
Copy link
Contributor

wez commented Mar 15, 2018

I'm also in favor of moving this function into rust-embedded/svd2rust#190 for the sake of having fewer things to review and merge :)

Copy link
Member

@Emilgardis Emilgardis left a comment

Choose a reason for hiding this comment

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

I've talked to @ryankurte and now this makes more sense to have in this crate. Only thing that needs to change is what @wez already adressed

@@ -178,6 +178,23 @@ pub struct Peripheral {
}

impl Peripheral {
pub fn derive_from(&self, other: &Peripheral) -> Peripheral {
let mut derived = self.clone();
if derived.group_name.is_none() && other.group_name.is_some() {
Copy link
Member

Choose a reason for hiding this comment

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

I second this. I'll merge this when this has been fixed.

bors bot added a commit that referenced this pull request Mar 17, 2018
52: edit to #49 r=Emilgardis a=Emilgardis
@Emilgardis Emilgardis dismissed their stale review March 17, 2018 05:48

implemented in #52

@bors bors bot merged commit e2e3ab4 into rust-embedded:master Mar 17, 2018
@Emilgardis
Copy link
Member

@nsabovic I took the liberty and implemented this in #52.

@nsabovic nsabovic deleted the derivedfrom branch March 17, 2018 23:24
@nsabovic
Copy link
Contributor Author

@Emilgardis —Thanks for the patience, and for fixing up my awkward style!

The fixed version has lines like:

derived.group_name = derived.group_name.or(other.group_name.clone())

Shouldn't it be:

derived.group_name = derived.group_name.or_else(|| other.group_name.clone())

To avoid calling clone() unconditionally, and then discarding it if derived already has the value?

@Emilgardis
Copy link
Member

@nsabovic Yes I should have done that. Right now master is in a frozen state as we are working on #46, I fixed this in bdfa727 however. Thank you for mentioning it! 👍

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.

3 participants