Skip to content

Rework usages/naming of configuration macros #2793

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
Apr 9, 2019
Merged

Rework usages/naming of configuration macros #2793

merged 1 commit into from
Apr 9, 2019

Conversation

galpeter
Copy link
Contributor

@galpeter galpeter commented Mar 8, 2019 •
edited
Loading

There are quite a few configuration macros in the project.
As discussed in the #2520 issue there are a few awkward constructs.

Main changes:

  • Renamed all CONFIG_DISABLE_<name>_BUILTIN macro to JERRY_BUILTIN_<name> format.
  • The special JERRY_BUILTINS macro specifies the basic config for all es5.1 builtins.
  • Renamed all CONFIG_DISABLE_ES2015_<name> to JERRY_ES2015_<name> format.
  • The special JERRY_ES2015 macro specifies the basic config for all es2015 builtins.
  • Renamed UNICODE_CASE_CONVERSION to JERRY_UNICODE_CASE_CONVERSION.
  • Renamed ENABLE_REGEXP_STRICT_MODE to JERRY_REGEXP_STRICT_MODE.
  • All options (in this change) can have a value of 0 or 1.
  • Renamed ENABLE_REGEXP_STRICT_MODE to JERRY_REGEXP_STRICT_MODE.
    JERRY_REGEXP_STRICT_MODE is set to 0 by default.
  • Reworked CONFIG_ECMA_NUMBER_TYPE macro to JERRY_NUMBER_TYPE_FLOAT64 name and now
    it uses the value 1 for 64 bit floating point numbers and 0 for 32 bit floating point
    number.
    By default the 64-bit floating point number mode is enabled.
  • All new JERRY_ defines can be used wit the #if ENABLED (JERRY_...) construct to
    test if the feature is enabled or not.
  • Added/replaced a few config.h includes to correctly propagate the macro values.
  • Added sanity checks for each macro to avoid incorrectly set values.
  • Updated profile documentation.
  • The CMake feature names are not updated at this point.

rerobika
rerobika reviewed Mar 18, 2019
View reviewed changes
jerry-core/api/jerry-snapshot.c Outdated
@@ -42,12 +42,12 @@ snapshot_get_global_flags (bool has_regex, /**< regex literal is present */

uint32_t flags = 0;

#ifndef CONFIG_DISABLE_REGEXP_BUILTIN
#if defined (JERRY_BUILTIN_REGEXP) && (JERRY_BUILTIN_REGEXP == 1)
Copy link
Member

Choose a reason for hiding this comment

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

As I see there is a sanity check macro guard for the incorrect configuration:

#if !defined (JERRY_BUILTIN_REGEXP) \
|| ((JERRY_BUILTIN_REGEXP != 0) && (JERRY_BUILTIN_REGEXP != 1))
# error "Invalid value for JERRY_BUILTIN_REGEXP macro."
#endif

Therefore wouldn't be much easier to use only the

#if JERRY_BUILTIN_REGEXP == 1
//code
#endif

guard all over the codebase? I think these double checks could make the code much harder to read especially if two or more guards are combined e.g..

@LaszloLango
Copy link
Contributor

Please rebase to the current master

@dbatyai
Copy link
Member

dbatyai commented Mar 18, 2019 •
edited
Loading

I agree with @rerobika, we should aim to make the code as readable as we can.

Since each guard is checked to have well defined values, I would even go as far as to say that we should skip the comparison checks as well (in case of boolean guards).

For example, instead of

#if (JERRY_A == 1) && (JERRY_B == 0)

we could simply write

#if JERRY_A && !JERRY_B

What do you think?

@galpeter
Copy link
Contributor Author

@dbatyai @rerobika I'm fine with simplifying (the sanity check would catch all problems).
Another thing I was considering is the format: #if ENABLED(JERRY_FEATURE) which could hide all defined and comparison checks.

Which one would you prefer?

@dbatyai
Copy link
Member

dbatyai commented Mar 18, 2019 •
edited
Loading

I think #if ENABLED(JERRY_FEATURE) is fine, it's more descriptive while remaining well readable.

@rerobika
Copy link
Member

The #if ENABLED(JERRY_FEATURE) is fine for me as well.

@galpeter
Copy link
Contributor Author

@rerobika @dbatyai I've just updated with the ENABLED version :)

@galpeter galpeter force-pushed the config_rework branch 2 times, most recently from f867fb2 to 6d1712d Compare March 18, 2019 18:30
rerobika
rerobika reviewed Mar 18, 2019
View reviewed changes
jerry-core/config.h
@@ -107,4 +189,128 @@
*/
#define CONFIG_ECMA_GC_NEW_OBJECTS_SHARE_TO_START_GC (16)


/**
* Sanity check for macros to see if the values are 0 or 1
Copy link
Member

Choose a reason for hiding this comment

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

Are these sanity checks are generated somehow, or they should be written by hand? If the second, it would be good to add a note for add these checks whenever a new feature guard is introduced.

Copy link
Member

Choose a reason for hiding this comment

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

Also, as the feature guards are grouped by categories (es5.1, es2015, other), I think these checks should be separated as well for the easier maintenance. (Probably a comment is enough)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add comments for these.

@zherczeg zherczeg mentioned this pull request Mar 19, 2019
Closed
LaszloLango
LaszloLango approved these changes Mar 25, 2019
View reviewed changes
Copy link
Contributor

@LaszloLango LaszloLango left a comment

Choose a reason for hiding this comment

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

LGTM

@galpeter galpeter force-pushed the config_rework branch 2 times, most recently from 14e8519 to de49acd Compare April 5, 2019 12:08
@galpeter
Rework usages/naming of configuration macros [part 1]
8cbbbea 
There are quite a few configuration macros in the project.
As discussed in the jerryscript-project#2520 issue there are a few awkward constructs.

Main changes:

* Renamed all CONFIG_DISABLE_<name>_BUILTIN macro to JERRY_BUILTIN_<name> format.
* The special JERRY_BUILTINS macro specifies the basic config for all es5.1 builtins.
* Renamed all CONFIG_DISABLE_ES2015_<name> to JERRY_ES2015_<name> format.
* The special JERRY_ES2015 macro specifies the basic config for all es2015 builtins.
* Renamed UNICODE_CASE_CONVERSION to JERRY_UNICODE_CASE_CONVERSION.
* Renamed ENABLE_REGEXP_STRICT_MODE to JERRY_REGEXP_STRICT_MODE.
* All options (in this change) can have a value of 0 or 1.
* Renamed ENABLE_REGEXP_STRICT_MODE to JERRY_REGEXP_STRICT_MODE.
  JERRY_REGEXP_STRICT_MODE is set to 0 by default.
* Reworked CONFIG_ECMA_NUMBER_TYPE macro to JERRY_NUMBER_TYPE_FLOAT64 name and now
  it uses the value 1 for 64 bit floating point numbers and 0 for 32 bit floating point
  number.
  By default the 64-bit floating point number mode is enabled.
* All new JERRY_ defines can be used wit the `#if ENABLED (JERRY_...)` construct to
  test if the feature is enabled or not.
* Added/replaced a few config.h includes to correctly propagate the macro values.
* Added sanity checks for each macro to avoid incorrectly set values.
* Updated profile documentation.
* The CMake feature names are not updated at this point.

JerryScript-DCO-1.0-Signed-off-by: Peter Gal [email protected]
rerobika
rerobika approved these changes Apr 9, 2019
View reviewed changes
Copy link
Member

@rerobika rerobika left a comment

Choose a reason for hiding this comment

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

LGTM

@rerobika rerobika merged commit 40f7b1c into jerryscript-project:master Apr 9, 2019
@rerobika rerobika mentioned this pull request Apr 9, 2019
Merged
@akosthekiss akosthekiss mentioned this pull request Sep 13, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LaszloLango LaszloLango LaszloLango approved these changes

@rerobika rerobika rerobika approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@galpeter @LaszloLango @dbatyai @rerobika