Skip to content

Conversation

@zherczeg
Copy link
Member

@zherczeg zherczeg commented Jun 6, 2018

No description provided.

@zherczeg zherczeg mentioned this pull request Jun 6, 2018
@LaszloLango LaszloLango added this to the Release 2.0 milestone Jun 6, 2018
@LaszloLango LaszloLango added the api Related to the public API label Jun 6, 2018

- `mode` - operational mode, see [jerry_gc_mode_t](#jerry_gc_mode_t)

**Example**
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update the example as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. It runs with doctest as well now.


## jerry_gc_mode_t

Set garbage collection operational mode
Copy link
Member

Choose a reason for hiding this comment

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

I'm no native English speaker but isn't this "operation mode" or "operating mode"?

Copy link
Member Author

Choose a reason for hiding this comment

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

According to google, operation mode is used 262M sites, while operational mode 117M sites. Both are used by high profile (e.g. microsoft or google technical docs) sites. I think both are ok, so I don't want to change unless you insist on it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I checked Cambridge dictionary:

operation - noun
operational - adjective
operating - present participle

I would prefer operational here.

Copy link
Member Author

@zherczeg zherczeg Jun 8, 2018

Choose a reason for hiding this comment

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

Google: The present participle is a participle that ends in ing. It can be used with the auxilliary verb 'to be' to form the continuous tense. It always takes the 'ing' form of the verb, even irregular verbs have an '...ing' form, in fact virtually all English words that end with 'ing' are present participles.

I think operating mode is also correct, if we emphasize doing the garbage collection in specific a way. The operational mode simply emphasize the behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

E.g:
operating system - a system which is running (emphasizing continuity)
operational system - a system which is capable of running (not relevant if it is doing it now)
operation system - double noun, usually a phrase or not used at all. I think the latter is true here.

Copy link
Member

Choose a reason for hiding this comment

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

As you've said, saying something is 'operational' means it is capable of running. Saying that the mode is operational means that the thing that is running will be able to run in that mode, but also implies that a 'not operational' mode exists, which 'won't be able to run'. This is not the case, as the mode here means a specific a way of operation, and not whether the GC can actually run, or is running.

I'm not sure what behavior you'd like to emphasize by using operational mode, yet it feels instinctively wrong to me, and I'd say the suggestions made by @akosthekiss would be the the correct way to go.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't exactly say that. Tricky way of turning my words :)

Adjectives extends the meaning of words, while nouns do not. Think about the difference of "wooden house" and "tree house". The former uses an adjective to extend a noun, while the latter is simply a phrase.

I found "operational mode" in a well known English dictionary:
https://en.oxforddictionaries.com/definition/standby
An operational mode of an electrical appliance in which the power is switched on but the appliance is not actually functioning.

I could not find the same thing with "operation mode".

Copy link
Member

Choose a reason for hiding this comment

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

I've consulted an English teacher and operation was preferred over operational. But still no native speaker chiming in on either side. As this is no API function name but a doc text only, I wont be pushing further.


jerry_cleanup ();
}

Copy link
Member

Choose a reason for hiding this comment

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

Extra empty line.

jerry_gc ();
jerry_gc_mode_t mode = JERRY_GC_SEVERITY_LOW;

if (args_cnt > 0 && jerry_value_to_boolean (args_p[0]))
Copy link
Member

Choose a reason for hiding this comment

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

Because of this change, neither args_cnt nor args_p are unused anymore. (See above.)

if (args_cnt > 0 && jerry_value_to_boolean (args_p[0]))
{
mode = JERRY_GC_SEVERITY_HIGH;
}
Copy link
Member

Choose a reason for hiding this comment

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

This all could go into a ?: expression to avoid making the initialization of mode potentially a dead expression.

  jerry_gc_mode_t mode = args_cnt > 0 && jerry_value_to_boolean (args_p[0]) ? JERRY_GC_SEVERITY_HIGH
                                                                            : JERRY_GC_SEVERITY_LOW;

Copy link
Member

Choose a reason for hiding this comment

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

BTW, the doc string of the function should be updated describing how it deals with its arguments. Something like:

The routine invokes the GC with high severity if it has at least one argument and the first argument is equivalent to boolean true; it calls the GC with low severity otherwise.

@akosthekiss akosthekiss added the jerry-ext Related to the jerry-ext library label Jun 7, 2018
JerryScript-DCO-1.0-Signed-off-by: Zoltan Herczeg zherczeg.u-szeged@partner.samsung.com
@zherczeg
Copy link
Member Author

zherczeg commented Jun 8, 2018

Patch updated.

@zherczeg
Copy link
Member Author

Any more comments for this patch?

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

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.

LGTM

@yichoi yichoi merged commit 1044523 into jerryscript-project:master Jun 15, 2018
@zherczeg zherczeg deleted the gc_api branch June 18, 2018 06:19
@rerobika rerobika mentioned this pull request Jul 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api Related to the public API jerry-ext Related to the jerry-ext library

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants