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

add stream_fifo: fifo_v3 with rdy/vld interface #74

Merged
merged 10 commits into from
Apr 9, 2020
Merged

add stream_fifo: fifo_v3 with rdy/vld interface #74

merged 10 commits into from
Apr 9, 2020

Conversation

da-gazzi
Copy link
Contributor

Just a trivial wrapper for fifo_v3 - I found it handy to have around.

Copy link
Contributor

@zarubaf zarubaf left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. I have a couple of comments and requests for changes. We adhere to the SystemVerilog coding style of lowrisc. You might want to look into this as well.

da-gazzi and others added 3 commits March 25, 2020 13:52
Co-Authored-By: Florian Zaruba <[email protected]>
Co-Authored-By: Florian Zaruba <[email protected]>
Co-Authored-By: Florian Zaruba <[email protected]>
@zarubaf
Copy link
Contributor

zarubaf commented Mar 25, 2020

Could you also add an entry in the CHANGELOG.md, to Bender.yml and to the README.md please. Thanks!

@da-gazzi
Copy link
Contributor Author

I fixed most of your requests - the alignment is still a bit out of whack comparing to other modules in this repo; this is due to me using a different editor (and not being able to get sublime text to make it look somewhat reasonable).

@andreaskurth
Copy link
Contributor

andreaskurth commented Mar 30, 2020

Thanks for your PR, @da-gazzi, this is certainly a useful addition!

the alignment is still a bit out of whack comparing to other modules in this repo; this is due to me using a different editor (and not being able to get sublime text to make it look somewhat reasonable).

I do not see how your editor has more influence on exactly which characters you put into a file than you do. Every plain text editor I have used allows me to exactly set individual ASCII characters. ;-)

I took the liberty to add a few commits that make this PR compliant to our coding conventions. Most importantly, never use tabs. You can pull those commits from the stream_fifo branch (I pushed them there since I did not want -- and probably would not be able -- to push to your master).

Sorting source files of the same level alphabetically means that each
file has a well-defined place.  This makes operations such as `diff`ing
two file lists trivial.
@da-gazzi
Copy link
Contributor Author

da-gazzi commented Apr 4, 2020

Thanks for your PR, @da-gazzi, this is certainly a useful addition!

the alignment is still a bit out of whack comparing to other modules in this repo; this is due to me using a different editor (and not being able to get sublime text to make it look somewhat reasonable).

I do not see how your editor has more influence on exactly which characters you put into a file than you do. Every plain text editor I have used allows me to exactly set individual ASCII characters. ;-)

I took the liberty to add a few commits that make this PR compliant to our coding conventions. Most importantly, never use tabs. You can pull those commits from the stream_fifo branch (I pushed them there since I did not want -- and probably would not be able -- to push to your master).

Thanks for the fixes! I have applied and pushed them to my fork. It should now be ready for merging.

What I meant by my remark regarding editors was that my editor does not "auto-indent" things the same way e.g. sublime text does, which I assume to be the editor of choice for all the prominent contributors to these repositories. Of course I can manually put spaces wherever I please, but in the interest of my own mental well-being I avoid this. I go with whatever the editor I'm using does by default. I will do my best to adhere to the style guide in future pull requests, of course ;)

By the way, it seems the tabs were actually added by sublime text in my unsuccessful attempts to find an easy way to automatically "conformalize" the code style - I know I won't do that again!

@andreaskurth
Copy link
Contributor

andreaskurth commented Apr 6, 2020

Thanks for the fixes! I have applied and pushed them to my fork. It should now be ready for merging.

Thanks, LGTM.

What I meant by my remark regarding editors was that my editor does not "auto-indent" things the same way e.g. sublime text does, which I assume to be the editor of choice for all the prominent contributors to these repositories. Of course I can manually put spaces wherever I please, but in the interest of my own mental well-being I avoid this. I go with whatever the editor I'm using does by default. I will do my best to adhere to the style guide in future pull requests, of course ;)

AFAIK, most of us use different editors (Sublime, Vim, VS Code, ..), but plugins such as EditorConfig make it easy to automatically have a consistent style. Which reminds me: Should we add a .editorconfig to this repo, @zarubaf?

@zarubaf
Copy link
Contributor

zarubaf commented Apr 9, 2020

Thanks. Yes, let's add the editorconfig in a separate PR. I'll open an issue for that.

@zarubaf zarubaf merged commit 953d454 into pulp-platform:master Apr 9, 2020
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