-
Notifications
You must be signed in to change notification settings - Fork 50
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
Test memory loading #206
Test memory loading #206
Conversation
- Added memory test resource files - Tests now use resource files - Complex test does not use suffixes on mem files - Other test does use suffixes - Both tests now actually test values in addition to printing them out
- Use nicer interface for loadMemory from chisel.util
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.
This looks good, @chick. Very cool to see this in action.
Only nits and one test, ClockCrossingSpec.scala
, that seems to have been accidentally committed?
src/test/resources/mem3.txt
Outdated
1 | ||
0 | ||
1 | ||
|
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.
[super-nit] extra newline
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.
Removed, I think I added it playing around to see if it caused a problem
import firrtl.FileUtils | ||
import org.scalatest.{FreeSpec, Matchers} | ||
|
||
//noinspection TypeAnnotation |
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.
[nit] I'm against letting editor-specific settings/comments leak into mainline code when I expect that this could be accomplished with a user-local setting. It creates confusion unless you know what this does. I'd be similarly against Emacs file variables.
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.
I don't think this is an editor specific setting, it is a standard directive to the scala style-checker which we are supposed to be supporting.
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.
See what you think. I had never seen this before. From what I could dig up online, I concluded that it was IntelliJ adding code comments:
- https://stackoverflow.com/questions/7798908/disable-warning-in-intellij-for-one-line
- https://intellij-support.jetbrains.com/hc/en-us/community/posts/206280839--SuppressWarnings-versus-noinspection
Scalastyle directives (which I thought, perhaps incorrectly, all started with scalastyle
) I'm fine with, on the other hand.
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 are right, I didn't think it through I will change it to the scalastyle directive, sorry.
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.
I'll just get rid of it for now. It's an unfortunate scalastyle collision that I don't think there is a great way to fix. I'm not sure that we should change the style rules for this situation because other than IO I think this is a good thing to flag.
- Fixes based on review - remove extra newline at EOF from mem3 input file - remove un-related and leaked file: ClockCrossingSpec - change imports to reflect this moving to experimental - remove extra license line - remove //noinspection, in general use //scalastyle, here use nothing - bump scala version so it will compile with rest of stack
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
* Add tests for clock crossing and memory loading * LoadMemory test examples - Added memory test resource files - Tests now use resource files - Complex test does not use suffixes on mem files - Other test does use suffixes - Both tests now actually test values in addition to printing them out * LoadMemory test examples - Use nicer interface for loadMemory from chisel.util * Load memory from a file - Fixes based on review - remove extra newline at EOF from mem3 input file - remove un-related and leaked file: ClockCrossingSpec - change imports to reflect this moving to experimental - remove extra license line - remove //noinspection, in general use //scalastyle, here use nothing - bump scala version so it will compile with rest of stack (cherry picked from commit 7218873)
Adds functional tests for simple and complex memories being loaded from
file using $readmemh or $readmemb using binding files.