Skip to content

Replace vera with clang-format #4518

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 5, 2021
Merged

Replace vera with clang-format #4518

merged 1 commit into from
Nov 5, 2021

Conversation

rerobika
Copy link
Member

No description provided.

@rerobika rerobika added the tools Related to the tooling scripts label Jan 21, 2021
@rerobika rerobika changed the title Replace vera with clang-format [RFC] Replace vera with clang-format Jan 21, 2021
@rerobika
Copy link
Member Author

Note: depends on #4515.

Moreover, if the style change is accepted by the maintainers, I'll update the coding standards documentation.

akosthekiss
akosthekiss reviewed Jan 22, 2021
View reviewed changes
Copy link
Member

@akosthekiss akosthekiss left a comment

Choose a reason for hiding this comment

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

Some, feedback. I surely haven't exhaustively reviewed all changed files.

.clang-format Outdated
BreakBeforeBraces: Custom
BreakBeforeTernaryOperators: true
BreakStringLiterals: true
ColumnLimit: 120
Copy link
Member

Choose a reason for hiding this comment

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

This is indeed current style. Driving me crazy sometimes. Is this the right time to discuss it? (Or is this about mimicking existing formatting without any changes?)

Copy link
Member Author

Choose a reason for hiding this comment

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

It is the auto generated gnu-stlye descriptor with jerry-style modifications. I didn't take effort on unify the style in the descriptor but once the approach is accepted, I'll definitely update it as well.

@akosthekiss akosthekiss added the style Related to coding style label Jan 23, 2021
@lygstate
Copy link
Contributor

I like this idea, maybe we need python script to format files automatically

@rerobika rerobika force-pushed the clang_format branch 2 times, most recently from a79307e to f0ba049 Compare February 5, 2021 14:08
@rerobika rerobika force-pushed the clang_format branch 13 times, most recently from 10717ab to 515b387 Compare November 4, 2021 17:45
zherczeg
zherczeg reviewed Nov 4, 2021
View reviewed changes
jerry-core/api/jerry-snapshot.c
snapshot_buffer_size,
globals_p);
uint32_t offset =
static_snapshot_add_compiled_code (bytecode_p, snapshot_buffer_p, snapshot_buffer_size, globals_p);
Copy link
Member

Choose a reason for hiding this comment

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

This looks incorrect syntax.

Copy link
Member Author

Choose a reason for hiding this comment

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

No longer.

jerry-core/api/jerry-snapshot.c
@@ -535,8 +527,7 @@ jerry_snapshot_set_offsets (uint32_t *buffer_p, /**< buffer */
JERRY_ASSERT ((code_size % sizeof (uint32_t)) == 0);
buffer_p += code_size / sizeof (uint32_t);
size -= code_size;
}
while (size > 0);
} while (size > 0);
Copy link
Member

Choose a reason for hiding this comment

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

This looks incorrect syntax.

jerry-core/api/jerry-snapshot.c
@@ -439,8 +432,7 @@ static_snapshot_add_compiled_code (const ecma_compiled_code_t *compiled_code_p,

while (literal_start_p < (ecma_value_t *) buffer_p)
{
if (!ecma_is_value_direct_string (*literal_start_p)
&& !ecma_is_value_empty (*literal_start_p))
if (!ecma_is_value_direct_string (*literal_start_p) && !ecma_is_value_empty (*literal_start_p))
Copy link
Member

Choose a reason for hiding this comment

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

Why this is moved to one line? These are often harder to read.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since it fits into one line.

jerry-core/api/jerry-snapshot.c
@@ -928,8 +905,7 @@ jerry_exec_snapshot (const uint32_t *snapshot_p, /**< snapshot */

const jerry_snapshot_header_t *header_p = (const jerry_snapshot_header_t *) snapshot_data_p;

if (header_p->magic != JERRY_SNAPSHOT_MAGIC
|| header_p->version != JERRY_SNAPSHOT_VERSION
if (header_p->magic != JERRY_SNAPSHOT_MAGIC || header_p->version != JERRY_SNAPSHOT_VERSION
|| !snapshot_check_global_flags (header_p->global_flags))
Copy link
Member

Choose a reason for hiding this comment

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

Incorrect syntax.

jerry-core/ecma/base/ecma-alloc.c
@@ -99,7 +101,7 @@ ecma_dealloc_object (ecma_object_t *object_p) /**< object to be freed */
*
* @return pointer to allocated memory
*/
extern inline ecma_extended_object_t * JERRY_ATTR_ALWAYS_INLINE
extern inline ecma_extended_object_t *JERRY_ATTR_ALWAYS_INLINE
Copy link
Member

Choose a reason for hiding this comment

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

Incorrect syntax.

Copy link
Member Author

@rerobika rerobika Nov 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

since it's a marco, not a real __attribute it won't be aligned as before.

jerry-core/ecma/base/ecma-errors.c
@@ -20,114 +20,114 @@
/**
* Error message, if an argument is has an error flag
*/
const char * const ecma_error_value_msg_p = "Argument cannot be marked as error";
const char* const ecma_error_value_msg_p = "Argument cannot be marked as error";
Copy link
Member

Choose a reason for hiding this comment

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

Incorrect syntax.

jerry-core/ecma/base/ecma-extended-info.c
#include "ecma-extended-info.h"

#include "ecma-helpers.h"

Copy link
Member

Choose a reason for hiding this comment

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

These newlines are unnecessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's valid.
Include order separated to incluce blocks:

  1. own header
  2. system header
  3. jerry* header
  4. ecma* header
  5. everything else.

jerry-core/ecma/base/ecma-helpers-conversion.c

#if defined (__GNUC__) || defined (__clang__)
#define ECMA_UINT128_DIV10(name) \
{ \
Copy link
Member

Choose a reason for hiding this comment

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

Incorrect syntax, the slash should be kept in its place.

Copy link
Member Author

Choose a reason for hiding this comment

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

Slash is aligned no the longest line of the macro, so still correct.

@rerobika rerobika force-pushed the clang_format branch 6 times, most recently from dadafd9 to 6db5106 Compare November 5, 2021 11:38
Replace vera++ with clang-format
25b5003 
JerryScript-DCO-1.0-Signed-off-by: Robert Fancsik [email protected]
@rerobika rerobika changed the title [RFC] Replace vera with clang-format Replace vera with clang-format Nov 5, 2021
@rerobika
Copy link
Member Author

rerobika commented Nov 5, 2021

The patch is ready, the 08.CODING-STARDARDS.MD needs to be updated in a follow up patch.

dbatyai
dbatyai approved these changes Nov 5, 2021
View reviewed changes
Copy link
Member

@dbatyai dbatyai left a comment

Choose a reason for hiding this comment

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

LGTM

robertsipka
robertsipka approved these changes Nov 5, 2021
View reviewed changes
Copy link
Contributor

@robertsipka robertsipka left a comment

Choose a reason for hiding this comment

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

LGTM

@robertsipka robertsipka merged commit badfdf4 into jerryscript-project:master Nov 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zherczeg zherczeg zherczeg left review comments

@akosthekiss akosthekiss akosthekiss left review comments

@robertsipka robertsipka robertsipka approved these changes

@dbatyai dbatyai dbatyai approved these changes

Assignees
No one assigned
Labels
style Related to coding style tools Related to the tooling scripts
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@rerobika @lygstate @zherczeg @robertsipka @akosthekiss @dbatyai