Skip to content

One enum#358

Merged
bors[bot] merged 1 commit into
rust-embedded:masterfrom
burrbull:oneenum
Jul 31, 2019
Merged

One enum#358
bors[bot] merged 1 commit into
rust-embedded:masterfrom
burrbull:oneenum

Conversation

@burrbull

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
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
Copy Markdown
Contributor

bors try

bors Bot added a commit that referenced this pull request Jul 30, 2019
@burrbull

Copy link
Copy Markdown
Member Author

It just can not be.

@therealprof

Copy link
Copy Markdown
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

burrbull commented Jul 30, 2019

Copy link
Copy Markdown
Member Author

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

@therealprof

therealprof commented Jul 30, 2019

Copy link
Copy Markdown
Contributor

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

bors Bot commented Jul 30, 2019

Copy link
Copy Markdown
Contributor

try

Build succeeded

@burrbull

Copy link
Copy Markdown
Member Author

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

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

@therealprof

Copy link
Copy Markdown
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
Copy Markdown
Member Author

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

@therealprof

Copy link
Copy Markdown
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
Copy Markdown
Member Author

cc @adamgreig
cc @Disasm

@Disasm

Disasm commented Jul 31, 2019

Copy link
Copy Markdown
Member

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

therealprof commented Jul 31, 2019

Copy link
Copy Markdown
Contributor

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

Disasm commented Jul 31, 2019

Copy link
Copy Markdown
Member

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

@burrbull

Copy link
Copy Markdown
Member Author

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

@Disasm

Disasm commented Jul 31, 2019

Copy link
Copy Markdown
Member

@burrbull Checked, the error is still here.

@burrbull

Copy link
Copy Markdown
Member Author

Try again, please. Now it should be fixed.

@Disasm

Disasm commented Jul 31, 2019

Copy link
Copy Markdown
Member

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

@therealprof

Copy link
Copy Markdown
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

Disasm commented Jul 31, 2019

Copy link
Copy Markdown
Member

@therealprof Yep, working on this.

@therealprof

Copy link
Copy Markdown
Contributor

@burrbull Would you rebase please?

@burrbull

Copy link
Copy Markdown
Member Author

Done.

@therealprof therealprof left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM.

@therealprof

Copy link
Copy Markdown
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 <zgarbul.andrey@gmail.com>
@bors

bors Bot commented Jul 31, 2019

Copy link
Copy Markdown
Contributor

Build succeeded

@bors bors Bot merged commit 5ccfc14 into rust-embedded:master Jul 31, 2019
@burrbull

Copy link
Copy Markdown
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