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

Write the coding-style guide for Modmesh #362

Closed
wants to merge 7 commits into from
Closed
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions contrib/CODINGSTYLE.md
Copy link
Member

Choose a reason for hiding this comment

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

Please rename to /CODINGSTYLE.md. The directory contrib/ is for code not for how to write code.

Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
# Coding Style
## Introduction
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this section whose only content is the table of contents.

This is the coding style guide for Modmesh.
* [Introduction](#introduction)
* [Naming](#naming)
* [Comments](#comments)
* [Formatting](#formatting)
* [Class](#class)
* [Functions](#functions)

### Naming
1. The class names use Camel case, such as ```class R3DWidget;```.
Copy link
Collaborator

Choose a reason for hiding this comment

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

R3DWidget is not a good example. QT related and non-QT can be different.
Also, let's clarify if we want HTTPRequest or HttpReuqest.

Copy link
Member

Choose a reason for hiding this comment

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

Correct. Please distinguish Qt and non-Qt classes. If not mentioned, the style refers to non-Qt classes, and you should take non-Qt classes for example.

Copy link
Collaborator Author

@ThreeMonth03 ThreeMonth03 Jun 16, 2024

Choose a reason for hiding this comment

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

R3DWidget is not a good example. QT related and non-QT can be different. Also, let's clarify if we want HTTPRequest or HttpReuqest.

Sorry, I don't get it.
How does the cpp program call the HTTPRequest or HttpReuqest function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

R3DWidget is not a good example. QT related and non-QT can be different. Also, let's clarify if we want HTTPRequest or HttpReuqest.

By the way, do you mean that if the class use HTTPRequest or HttpReuqest,
the class belongs to a QT-related classes?

Copy link
Member

Choose a reason for hiding this comment

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

R* classes are Qt classes in modmesh. I name them with the leading R because it's the next letter of Q. I do not want to give them the leading letter Q which is already used by Qt itself.

R3DWidget is a modmesh-made Qt derived class. The CamelCase rule is ambiguous for it because "3D" is an acronym for "three-dimensional" but "3" cannot be capitalized. Both R3DWidget and R3dWidget are CamelCased.

@tigercosmos used HTTPRequest and HttpRequest to explain that you need to consider how to deal with acronyms. The former keeps each of the letter in the acronym "HTTP" in upper-case. The latter treats the whole acronym as a word, and makes it "Http".

Treating an acronym as a word will make naming more consistent. So for acronyms, let's go with HttpRequest.

2. The name of the variable / using-declarations uses snake case,
such as ```R3DWidget * viewer;``` and ```using size_type = std::size_t;```.
Copy link
Member

Choose a reason for hiding this comment

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

Please use perfect indentation of markdown. And "```" is rst, not markdown.

3. The QT-related function name uses Camel case,
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's the difference with point 1.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I notice that whether the class is related QT, the naming of the class is Camel case.

Copy link
Member

Choose a reason for hiding this comment

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

Class names are mostly CamelCase, although exceptions are not rare when it comes to a helper class.

Copy link
Collaborator Author

@ThreeMonth03 ThreeMonth03 Jun 16, 2024

Choose a reason for hiding this comment

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

Class names are mostly CamelCase, although exceptions are not rare when it comes to a helper class.

What is the style of the helper class?
Should I mention the helper class in this manual?

Copy link
Member

Choose a reason for hiding this comment

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

It's complicated

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's complicated

Ok, we may skip the helper class temporaily.

whereas non-QT-related function name uses snake case.
4. The name of the class member starts with ```m_```.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
4. The name of the class member starts with ```m_```.
4. The name of the class member starts with `m_`.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What is the difference between ` and ```?

Copy link
Collaborator

Choose a reason for hiding this comment

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

` is for inline code, ``` is for code block

Copy link
Member

Choose a reason for hiding this comment

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

5. The name of [macros](https://en.cppreference.com/w/cpp/preprocessor/replace) starts with ```DECL_MM_```.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

According to @tigercosmos and my suggestion,
here is the brief and rough conclusion.

However, it seems that the naming of the macros is inconsistent.
image
Is there any naming rule about macros?

Copy link
Collaborator

Choose a reason for hiding this comment

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

interesting, @yungyuc could you decide a prefix name and let's change all the other?

Copy link
Member

Choose a reason for hiding this comment

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

Let's use MM_DECL*.

6. The [using-declarations](https://en.cppreference.com/w/cpp/language/using_declaration) ends with ```_type```.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
6. The [using-declarations](https://en.cppreference.com/w/cpp/language/using_declaration) ends with ```_type```.
6. The [using-declarations](https://en.cppreference.com/w/cpp/language/using_declaration) ends with `_type`.


### Comments
1. The comment blocks follow the [doxygen styles](https://www.doxygen.nl/manual/docblocks.html).
2. The one-line comment, ```/* end of NamespaceOrClassName */```,
should be appended to the end of a namespace or class.
For example :
Copy link
Member

Choose a reason for hiding this comment

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

Use a blank line between section, paragraph, and literal blocks.

```c++
namespace modmesh
{
// More code...
}; /* end of modmesh */
```
3. It is highly recommended to add the reference in the comment block.

### Formatting
1. It is necessary to obey the [clang-format](https://clang.llvm.org/docs/index.html), or you must fail the github action.
Copy link
Member

Choose a reason for hiding this comment

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

clang-format is enforced by CI already. I don't think we need to write it down in the style guide.

2. There exists a space between the definition of the classes/functions.
Copy link
Member

Choose a reason for hiding this comment

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

Do you mean a blank line by "space"?

3. A long line should be devided to several shorter lines.
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's the recommended length? 120?

Copy link
Member

Choose a reason for hiding this comment

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

No, we have not been able to decide what is a proper length. @ThreeMonth03 , could you please make it explicit that we do not yet have a set limitation on line width?

Copy link
Collaborator

Choose a reason for hiding this comment

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

note that for python code, the length limit is 80 for sure.


### Class
1. The class should follow [the rule of five](https://en.cppreference.com/w/cpp/language/rule_of_three).
Copy link
Member

Choose a reason for hiding this comment

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

The rule of five is not exactly a coding style. Or do you mean to explicit write = default and = delete as much as possible?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, that's what I meant.


### Functions
1. The recursive function should be replaced with the iterative function, unless it is inevitable to be used.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should I add more concrete example or basic suggestion(like const member function/ copy by const reference) for the items?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know we have such a rule.

Copy link
Member

Choose a reason for hiding this comment

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

This does not look like a coding style. It is a performance (HPC) concern. It should not be included in this file.

Loading