-
Notifications
You must be signed in to change notification settings - Fork 682
Add the JerryScript Coding Standards #1459
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
Conversation
docs/07.CODING-GUIDELINES.md
Outdated
``` | ||
|
||
All types, constants, and functions require a description in | ||
JerryScript. These comments should start with /**. The starting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The /**.
and /**
will make the string between them bold, so what you wanted to write disappears.
example: /. my text / in raw: /**. my text /**
.
IMHO we should but such special characters in the description between backticks (`).
docs/07.CODING-GUIDELINES.md
Outdated
part of a comment should focus on explaing "why" rather than | ||
"what". | ||
|
||
```diff |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using "diff entries" as separator seems weird for me. Can't we just use a simple header style?
docs/07.CODING-GUIDELINES.md
Outdated
|
||
## Conditional preprocessor directives | ||
|
||
A comment is required after #else and #endif in JerryScript. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO: #else and #endif should be between backticks so they will appear as inline code. (Also applies to keywords and any other code related pars in the descriptions)
4662b84
to
ea0478e
Compare
LGTM |
c705b73
to
31e4639
Compare
@LaszloLango added an extra section about expressions. Please check the patch again. |
31e4639
to
551b005
Compare
docs/07.CODING-GUIDELINES.md
Outdated
part of a comment should focus on explaing *why* rather than | ||
*what*. | ||
|
||
```diff |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not a simple header, like this?
Good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not visible enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @galpeter, can't we have a standard header that is visible enough? E.g. change the color of the font?
Right now it seems you are using code blocks for the headers which seems suboptimal :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a first round of reviewing, thanks for taking the initiative on this!
docs/07.CODING-GUIDELINES.md
Outdated
@@ -0,0 +1,679 @@ | |||
# Coding style guidelines for the JerryScript project |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about changing this to "JerryScript Coding Standards" + changing the filename to "07.CODING-STANDARDS"? "Coding Standards" seems to be the most common term for this kind of document (vs. coding style guide, coding conventions, etc.).
docs/07.CODING-GUIDELINES.md
Outdated
@@ -0,0 +1,679 @@ | |||
# Coding style guidelines for the JerryScript project | |||
|
|||
This text is a brief overview of JerryScript coding style. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JerryScript coding style -> JerryScript Coding Standards
docs/07.CODING-GUIDELINES.md
Outdated
|
||
* Indentation is two spaces. | ||
* Tab characters are not allowed. | ||
* Maximum line length is 120 characters long (excluding newline). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-long
docs/07.CODING-GUIDELINES.md
Outdated
* Indentation is two spaces. | ||
* Tab characters are not allowed. | ||
* Maximum line length is 120 characters long (excluding newline). | ||
* No trailing white spaces are allowed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
white spaces are allowed -> white space is allowed
docs/07.CODING-GUIDELINES.md
Outdated
* Tab characters are not allowed. | ||
* Maximum line length is 120 characters long (excluding newline). | ||
* No trailing white spaces are allowed. | ||
* Run `tools/run-tests.py --check-vera` to check majority of the style rules. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Majority seems a bit of a stretch :) How about "Run tools/run-tests.py --check-vera
to check several of the coding conventions automatically."?
docs/07.CODING-GUIDELINES.md
Outdated
|
||
Each type in jerryscript must be a lowercase string | ||
which is ended by `_t` characters. Furthermore each type | ||
declaration must be preceeded by a short description |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
preceeded -> preceded
docs/07.CODING-GUIDELINES.md
Outdated
Each type in jerryscript must be a lowercase string | ||
which is ended by `_t` characters. Furthermore each type | ||
declaration must be preceeded by a short description | ||
about the type and each field must have a short description |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
about -> of
docs/07.CODING-GUIDELINES.md
Outdated
|
||
```c | ||
/** | ||
* Short description about the following structure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
about -> of
docs/07.CODING-GUIDELINES.md
Outdated
typedef struct | ||
{ | ||
/* Field descriptions do not start with capital letters | ||
* and there is no full stops at the end. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stops -> stop
docs/07.CODING-GUIDELINES.md
Outdated
## Function declarations | ||
|
||
Function declarations in JerryScript are verbose but this format | ||
reduces maintenance costs and allows faster understanding of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reduces maintenance costs -> reduces the maintenance cost
5ffd972
to
e694af9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing my feedback :) Did a second round of reviewing.
is doing. | ||
|
||
```diff | ||
+++ Good +++ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you investigate better ways to format this with Markdown?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. According to several sources github markdown does not support colors. I wish to keep this format.
docs/07.CODING-STANDARDS.md
Outdated
|
||
All types, constants and functions require a description in | ||
JerryScript. These comments should start with `/**`. The starting | ||
`/**` and ending `*/` must be in separate line. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in separate line -> on separate lines
docs/07.CODING-STANDARDS.md
Outdated
## Preprocessor defines | ||
|
||
The name of a preprocessor macro must be an uppercase string | ||
and these macros must be preceeded by a description. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
preceeded -> preceded
docs/07.CODING-STANDARDS.md
Outdated
* Only a single newline separator is allowed. | ||
* Source files must be terminated by a newline. | ||
* Global declarations must be separated by a newline. | ||
* Newlines are not allowed after an open brace or before a close brace. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
open brace or before a close brace -> opening curly brace or before a closing curly brace
docs/07.CODING-STANDARDS.md
Outdated
* Source files must be terminated by a newline. | ||
* Global declarations must be separated by a newline. | ||
* Newlines are not allowed after an open brace or before a close brace. | ||
* No newlines are allowed between control staments (if-else, while, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
staments -> statements
b, | ||
c); | ||
``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function_c (a,
b,
c);
function_c (a,
b,
function_c (a,
b,
c);
Should be moved to the bad section :)
docs/07.CODING-STANDARDS.md
Outdated
|
||
## Variable declarations | ||
|
||
JerryScript requires C99 standard support so variable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JerryScript requires C99 standard support -> JerryScript is a pure C99 codebase
docs/07.CODING-STANDARDS.md
Outdated
* declarations. A reviewer might request such a change. */ | ||
int ***int_p; | ||
|
||
/* This rule true for type casting as well. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This rule true for -> This rule applies for
docs/07.CODING-STANDARDS.md
Outdated
|
||
## Types | ||
|
||
Each type in jerryscript must be a lowercase string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jerryscript -> JerryScript
docs/07.CODING-STANDARDS.md
Outdated
## Types | ||
|
||
Each type in jerryscript must be a lowercase string | ||
which is which ends with a `_t` suffix. Furthermore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the "which is".
28d8b1d
to
f302710
Compare
JerryScript-DCO-1.0-Signed-off-by: Zoltan Herczeg [email protected]
f302710
to
826f98a
Compare
What's the right style if a function call statement is exceed 120 limit. Only one actual parameter in the function
The above thing will happen if the names of ret, function and arg are too long. |
I'd say that you should use shorter function and variable names. :) IMHO too long names are as bad as too short ones. But this is only my personal preference. |
Perhaps we should land this PR sometimes. |
Still LGTM from my side. |
LGTM |
Do we need more discussion? |
I have no more to add. |
Ok. Landing this. if needed, we can duscuss further. |
No description provided.