lowRISC / style-guides

lowRISC Style Guides
Creative Commons Attribution 4.0 International
359 stars 122 forks source link

Add guidance on functions and tasks #29

Closed GregAC closed 4 years ago

GregAC commented 4 years ago

Fixes #28

I've gone beyond the scope of what we've discussed in #28 in terms of rules/recommendations here. I am happy for these to be challenged just things that came up whilst I was writing it that I felt needing addressing. A brief list of what I've added on top if the #28 discussion is:

imphil commented 4 years ago

@GregAC Looks like this PR is almost there, can you incorporate the last suggestions from Scott?

GregAC commented 4 years ago

Yup don't worry haven't forgotten about it.

I think the only real point of discussion (beyond corrections, small improvements/additions etc) was how restrictive we want to be on what functions can take as arguments.

I'd quite purposefully gone for a function as a stand-in for a block of combinational logic style and said input only via arguments with only one output.

Multiple outputs are useful but of course still possible, you just need to concatenate things or use a struct as a return type. In my experience people generally don't do super-complex things with functions in RTL so I don't think disallowing output arguments will cause major issues.

I'll update the style guide today assuming we're happy with that idea but give the other commenters a chance to reply to continue the discussion if they still disagree with that style.

GregAC commented 4 years ago

Apologies for the slow updates, have now incorporated all of the feedback, though a couple of places where I disagreed, see the comments.

Thanks for the reviews @sriyerg @sjgitty @imphil @msfschaffner

eunchan commented 4 years ago

I also like this approach. Thanks! LGTM

GregAC commented 4 years ago

Hitting the merge button on this as I think everyone's more or less happy with it. If there is something people disagree with feel free to continue the discussion, we can always do another PR to change things.