Coding Style Guide #3026
Coding Style Guide #3026
Conversation
Explain when using auto is acceptable
|
@amin1377 Any comments on what should be added to this guide is appreciated. |
|
Fully agree with the usage of auto as documented here. I guess we also should talk about the naming schemes we use. What I personally extrapolated from the code base and remember currently is that Classes are PascalCase, everything else is snake_case, structs are trivial and C-like and start with t_ but it should all be written down. One more thing is saying that people should prioritize (but not blindly use in all circumstances) using vtr:: containers over STL and use the various LOG/ERROR macros over directly using printf/cout. |
There was a problem hiding this comment.
Hi Soheil, thank you so much for starting to add documentation on coding style! This is extremely important for such a large project such as VTR.
I think the coding style that we tell the students in ECE297 will be a good reference for a checklist of things to add to this documentation:
tutorial5_code_style.pdf
Specifically this slide:
I think the major things that are missing are:
- The variable, function, and classes naming conventions that we use in VTR
- Keeping function short (i.e. pulling out code into helper methods)
- Self-checking code (i.e. proper use of VTR_ASSERT)
I think the three above are very important to document since we can point people to this page if they raise a PR that does not have proper style; since I believe these three things (as well as the things you have documented) would block a PR from being merged in my opinion!
I know our goal is to keep this style guide brief; but if you would like more things to add to the style guide, I can point you to some of the different style guides that I am familiar with!
|
|
||
| Comment types and rules: | ||
|
|
||
| - Use `/* ... */` **only** for Doxygen documentation comments. |
There was a problem hiding this comment.
I'd weaken this a bit / explain the reasoning. Seem a bit strong on the /* vs. //
There was a problem hiding this comment.
I think having a uniform commenting style is helpful. This doesn’t forbid multiline comments. It just means implementation-related comments should start with //, including each line of a multiline comment. This makes it easy to distinguish implementation comments from documentation comments
| - Do not explain *what* the code is doing if it's already clear from the code itself. | ||
| - Keep comments up to date. Outdated comments are worse than no comments. | ||
| - Use Doxygen-style `/** ... */` or `///` for documenting APIs, classes, structs, members, and files. | ||
|
|
There was a problem hiding this comment.
Follow the instructions at <link> to add new utilities, APIs or internal functionality doxygen documentation to the html pages at <link>.
There was a problem hiding this comment.
I am not sure which page in the docs I should point to. Can you send the link here?
There was a problem hiding this comment.
first link: https://docs.verilogtorouting.org/en/latest/dev/code_documentation/
and
for second link, could do:
https://docs.verilogtorouting.org/en/latest/api/vpr/
(or just a generic link to the docs, which would end at /latest)
|
@soheilshahrouz We should also add a section on the use of header files. Specifically that "#pragma once" should be the first line in any header file (even before file comments). Through looking around online, for large projects it is recommended for "#pragma once" to be declared before the file comment as well as code. The idea is that the file comment will be duplicated in files that may include it multiple times, which for large projects can slow down the compilation speed due to IO. I am currently working on a PR that moves all include files to this style. |
|
I'd like to put a discussion of this on the vtr agenda for next week. Can I get you to walk through the style guide then? |
|
|
|
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| - Prefer easier-to-read language features and idioms. | ||
| - Avoid overusing advanced C++ features (e.g., template metaprogramming, operator overloading, SFINAE) unless they are essential. |
There was a problem hiding this comment.
My (unsolicited) opinion on this: I agree with the sentiment, but maybe the examples aren't very accurate. Operator overloading has been in C++ since 1998 and should be OK unless people want to + rr graphs together. Also the std::vector<int> example below relies on knowing the compiler would optimize out that copy. It could return a const reference, but if the underlying storage isn't exactly a vector, it makes sense to return a view or range with an auto return type. For instance, Tatum's interfaces are written assuming std::vector and I once gave up trying to make a parallel incremental analyzer based on tbb::concurrent_vector because of this. All in all, I think advanced and new features should be OK as long as they make things simpler.
I think it's good to advise against deep abstract class hierarchies here as it's really hard for tools to jump to the definition of a virtual function (another tatum thing but it may have examples in other places). The combination of templates and virtual functions is particularly hard to navigate. Maybe a piece of code trying to be generic should use one or the other, and not both (so don't do what uxsdcxx-generated code does. or NetlistRouter haha)
Addresses #2678