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

One enum #358

Merged
merged 1 commit into from
Jul 31, 2019
Merged

One enum #358

merged 1 commit into from
Jul 31, 2019

Conversation

burrbull
Copy link
Member

@burrbull burrbull requested a review from a team as a code owner July 30, 2019 12:21
@therealprof
Copy link
Contributor

Weird, again I see absolutely not change in the generated files for my test SVDs (except for the _{}W-> {}_W change that is). It strikes me odd that some large SVD files are not even exercising the code you're changing...

@therealprof
Copy link
Contributor

bors try

bors bot added a commit that referenced this pull request Jul 30, 2019
@burrbull
Copy link
Member Author

It just can not be.

@therealprof
Copy link
Contributor

Well, it is... I reverted {}_W line from your PR just so I am able to see trees in the forrest but after that change the diff is completely empty. I'm using the upstream STM32F042x.svd for my tests, BTW. Because then I can actually see the compiled code and sizes...

@burrbull
Copy link
Member Author

burrbull commented Jul 30, 2019

Does your SVD contain <enumeratedValues>?
Try https://stm32.agg.io/rs/stm32f0x2.svd.patched

@therealprof
Copy link
Contributor

therealprof commented Jul 30, 2019

I can certainly try other SVD files. Two problems here: I cannot test my code base against patched versions and we do not have test cases on the CI based on those, so if only patched SVDs exercise those features we're basically not testing those which is bad...

@bors
Copy link
Contributor

bors bot commented Jul 30, 2019

try

Build succeeded

@burrbull
Copy link
Member Author

I think this is record.
изображение

Previous time:
изображение

@therealprof
Copy link
Contributor

Not really, CI times vary quite a bit. We had similar "quick" builds e.g. here: https://travis-ci.org/rust-embedded/svd2rust/builds/564713886

@burrbull
Copy link
Member Author

So is here something blocking merge?
Do you want I revert "{}_W" or something else?

@therealprof
Copy link
Contributor

Well, as I said. I can't see any changes with my set-up infrastructure and I'd definitely like to be sure that this is tested, preferably in CI. So either someone can tell me which currently tested SVD file exercises this change or this will need to wait until I switch my tests to use the stm32-rs svd file.

@burrbull burrbull mentioned this pull request Jul 31, 2019
@burrbull
Copy link
Member Author

cc @adamgreig
cc @Disasm

@Disasm
Copy link
Member

Disasm commented Jul 31, 2019

Well, seems like the svd2rust output is unable to build on stable.

error: enum variants on type aliases are experimental
  --> src/aes/tag_in_flag.rs:75:22
   |
75 |         self.variant(TAG_IN_FLAG_A::CAN_INPUT)
   |                      ^^^^^^^^^^^^^^^^^^^^^^^^

Checked on this repo: https://github.com/riscv-rust/k210-pac

@therealprof
Copy link
Contributor

therealprof commented Jul 31, 2019

Oh, have I mentioned that we definitely should have SVD files in CI which exercises all code in a PR. 🙄

Well probably have to revert the variant changes and work on modernising CI a bit so this doesn't happen again.

@Disasm
Copy link
Member

Disasm commented Jul 31, 2019

Hmm, why VENDOR=OTHER is checked only on nightly?

@burrbull
Copy link
Member Author

@Disasm I forgot to try on stable. I've reverted. Can you try again.

@Disasm
Copy link
Member

Disasm commented Jul 31, 2019

@burrbull Checked, the error is still here.

@burrbull
Copy link
Member Author

Try again, please. Now it should be fixed.

@Disasm
Copy link
Member

Disasm commented Jul 31, 2019

Now it builds. On k210-pac it shows 6.3% build time reduction compared to master. Not bad!

@therealprof
Copy link
Contributor

@Disasm Since it seems to be buildable on stable, would you be willing to move RISC-V out of the OTHER category and make it testable on stable in a new PR?

@Disasm
Copy link
Member

Disasm commented Jul 31, 2019

@therealprof Yep, working on this.

@therealprof
Copy link
Contributor

@burrbull Would you rebase please?

@burrbull
Copy link
Member Author

Done.

Copy link
Contributor

@therealprof therealprof left a comment

Choose a reason for hiding this comment

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

LGTM.

@therealprof
Copy link
Contributor

bors r+

bors bot added a commit that referenced this pull request Jul 31, 2019
358: One enum r=therealprof a=burrbull

r? @therealprof 

Co-authored-by: Andrey Zgarbul <[email protected]>
@bors
Copy link
Contributor

bors bot commented Jul 31, 2019

Build succeeded

@bors bors bot merged commit 5ccfc14 into rust-embedded:master Jul 31, 2019
@burrbull
Copy link
Member Author

It's funny:
rust-lang/rust#61682

The problem that was blocking compilation on stable will not be in 1.37.

@burrbull burrbull deleted the oneenum branch August 1, 2019 14:35
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