search

lowRISC / style-guides

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

[doc] Three new optional styles added #32

Closed mwbranstad closed 4 years ago

mwbranstad commented 4 years ago

There are three new additional styles added to the verilog style guide. They are the following:

mwbranstad commented 4 years ago

Addresses issue #24

mwbranstad commented 4 years ago

Reduced scope to only the all caps topic. Used @sjgitty phrasing in update.

mwbranstad commented 4 years ago

After re-reviewing this PR, I have to admit that I agree with @vogelpi, in that adding an all caps option dilutes the original style. I am going to retract this PR. However, if I was around when the original discussion occurred, I would have pushed for all caps only, but that window has passed.

The style guide is generally quite good. I think a common style that promotes productivity and easier development is a must. The _q and _d notation is a long time favorite, and the module _i and _o is a recent favorite. I think the fsm guidelines are better than what I have used.

I still do not believe where the parenthesis sits on a module statement does not measure up to the above standard. The code is not better understood one way or another.

For boolean operators, I thought I was following the original intent with my example. Will leave this up to someone else to make it clearer.

mwbranstad commented 4 years ago

Hi Scott,

I still have this item open. I asked for three changes originally, but have given up on the first two (all caps parms, module format). I would like to know if I can code my control statements with Boolean operators. I think the issue is that I use assignment statements for control instead of if-then-else style.

I though you were going to propose new language to explain this. Please let me know what path you would like to take here.

BTW, I have noticed a number of “prim” modules that do not follow some of the guidelines. For example, the use of _d and _q for flop inputs and outputs is not used, a style I have used for many years.

From: Scott Johnson notifications@github.com Sent: Wednesday, April 1, 2020 1:30 PM To: lowRISC/style-guides style-guides@noreply.github.com Cc: Mark Branstad Mark.Branstad@wdc.com; Mention mention@noreply.github.com Subject: Re: [lowRISC/style-guides] [doc] Three new optional styles added (#32)

@sjgitty commented on this pull request.

In VerilogCodingStyle.mdhttps://github.com/lowRISC/style-guides/pull/32#discussion_r401815468:

@@ -965,12 +965,14 @@ facilitate the creation of IP that may be re-used across many projects.

The preferred naming convention for all immutable constants is to use ALL_CAPS, but there are times when the use of UpperCamelCase might be considered more natural.

+The naming convention for parameter and localparam allows for the use of ALL_CAPS or UpperCamelCase. The allowance for ALL_CAPS is to accommodate this ubiquitous style found in the ASIC industry, and by all third party IP vendors.

I have no problem with allowances for ALL_CAPS, but I think our code base is preferring UpperCamelCase, so that should be our leader. Plus you've undermined our past decisions a bit with the wording. This sentence plus the table below imply that ALL_CAPS is preferred, which isn't what we've agreed upon. How about this as a suggestion:

The naming convention for parameter and localparam allows for the use of ALL_CAPS or UpperCamelCase. UpperCamelCase is the preferred style for mutable parameters, but there is allowance for ALL_CAPS to accommodate the common style found in other code bases.

In VerilogCodingStyle.mdhttps://github.com/lowRISC/style-guides/pull/32#discussion_r401816080:

| Constant Type | Style Preference | Conversation |

| ---- | ---- | ---- |

| `define | ALL_CAPS | Truly constant |

-| module parameter | UpperCamelCase | truly modifiable by instantiation, not constant |

-| derived localparam | UpperCamelCase | while not modified directly, still tracks module parameter |

-| tuneable localparam | UpperCamelCase | while not expected to change upon final RTL version, is used by designer to explore the design space conveniently |

+| module parameter | ALL_CAPS or UpperCamelCase | truly modifiable by instantiation, not constant |

Please switch the order here since our code base is predominantly UpperCamelCase.

UpperCamelCase or ALL_CAPS

In VerilogCodingStyle.mdhttps://github.com/lowRISC/style-guides/pull/32#discussion_r401819064:

@@ -1390,10 +1392,14 @@ Use the Verilog-2001 combined port and I/O declaration style. Do not use the

Verilog-95 list style. The port declaration in the module statement should fully

declare the port name, type, and direction.

-The opening parenthesis should be on the same line as the module declaration,

-and the first port should be declared on the following line.

+The opening parenthesis may be on the same line as the module declaration,

I'm with @msfschaffnerhttps://github.com/msfschaffner on this one that it needs more discussion. We can spawn to a different PR or leave this one open for a while to get other opinions. @asbhttps://github.com/asb @imphilhttps://github.com/imphil @eunchanhttps://github.com/eunchan @tjaychenhttps://github.com/tjaychen @GregAChttps://github.com/GregAC @martin-luekerhttps://github.com/martin-lueker

I'm not an emacs user, so please weigh in if this is a true hardship. Even if it is hard to avoid, I don't believe it should be in our rationale explicitly as on the line below.

In VerilogCodingStyle.mdhttps://github.com/lowRISC/style-guides/pull/32#discussion_r401822600:

@@ -2385,9 +2407,10 @@ a check for X on the 4-state net before resolving to a 2-state variable.

Use logical constructs for logical comparisons, bit-wise for data.

Logical constructs (!, ||, &&, ==, !=) should be used for all

-constructs that are evaluating logic (true or false) values, such as

-if clauses and ternary assignments. Use bit-wise constructs (~, |,

-&, ^) for all data constructs, even if scalar.

+constructs that are evaluating control path logic. This includes boolean

+logic (true or false) values, such as if clauses and ternary assignments.

I have a recommended rewrite for this section in #21https://github.com/lowRISC/style-guides/issues/21, let's do that in a separate PR please. I can initiate that one. I feel this wording goes all the way to the other side of the current guidance, while #21https://github.com/lowRISC/style-guides/issues/21 was recommending allowing both styles in different contexts.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/lowRISC/style-guides/pull/32#pullrequestreview-385834498, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AMTDD3LYYWU3X7PDRTJ4OL3RKOBYVANCNFSM4LX26YCA.

msfschaffner commented 4 years ago

@mwbranstad, I am pretty sure that we still have code in our repo that does not fully adhere to the style guide. so if you come across something that you think is inconsistent you could either file an issue for that file, or if it is a quick and small fix, just make a PR for it. I am also cleaning up such things when I come across them...

mwbranstad commented 4 years ago

@msfschaffner ok thanks for the guidance.

sjgitty commented 4 years ago

Hi Scott, I still have this item open. I would like to know if I can code my control statements with Boolean operators. I think the issue is that I use assignment statements for control instead of if-then-else style. I though you were going to propose new language to explain this. Please let me know what path you would like to take here.

38 should address this @mwbranstad . Sorry for the delays.

BTW, I have noticed a number of “prim” modules that do not follow some of the guidelines. For example, the use of _d and _q for flop inputs and outputs is not used, a style I have used for many years.

Agreed. If you're feeling energetic, you can create a PR, otherwise feel free to file an issue and we'll pull it off the pile eventually. The issue should be in the opentitan repo.