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.

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

debugger: Disable code if not desired #1798

Merged
merged 1 commit into from
Nov 20, 2018
Merged

debugger: Disable code if not desired #1798

merged 1 commit into from
Nov 20, 2018

Conversation

rzr
Copy link
Contributor

@rzr rzr commented Nov 14, 2018

This part is not as critical for TizenRT,
but will help to make iotjs smaller.

Thanks-to: Daeyeon Jeong daeyeon.jeong@samsung.com
Relate-to: #1795
IoT.js-DCO-1.0-Signed-off-by: Philippe Coval p.coval@samsung.com

@rzr rzr force-pushed the sandbox/rzr/devel/review/master branch from b5999e2 to bb16678 Compare November 14, 2018 10:26
rzr added a commit to TizenTeam/iotjs that referenced this pull request Nov 14, 2018
@rzr
debugger: Disable code if not desired
bb16678 
This part is not as critical for TizenRT,
but will help to make iotjs smaller.

Thanks-to: Daeyeon Jeong <daeyeon.jeong@samsung.com>
Relate-to: jerryscript-project#1795
Forwarded: jerryscript-project#1798
IoT.js-DCO-1.0-Signed-off-by: Philippe Coval p.coval@samsung.com
@rzr rzr force-pushed the sandbox/rzr/devel/review/master branch from bb16678 to 8ddacdb Compare November 14, 2018 10:48
rzr added a commit to TizenTeam/iotjs that referenced this pull request Nov 14, 2018
@rzr
debugger: Disable code if not desired
8ddacdb 
This part is not as critical for TizenRT,
but will help to make iotjs smaller.

Thanks-to: Daeyeon Jeong daeyeon.jeong@samsung.com
Relate-to: jerryscript-project#1795
Forwarded: jerryscript-project#1798
IoT.js-DCO-1.0-Signed-off-by: Philippe Coval p.coval@samsung.com
@rzr rzr force-pushed the sandbox/rzr/devel/review/master branch from 8ddacdb to fe96267 Compare November 14, 2018 12:24
rzr added a commit to TizenTeam/iotjs that referenced this pull request Nov 14, 2018
@rzr
debugger: Disable code if not desired
fe96267 
This part is not as critical for TizenRT,
but will help to make iotjs smaller.

Thanks-to: Daeyeon Jeong daeyeon.jeong@samsung.com
Relate-to: jerryscript-project#1795
Forwarded: jerryscript-project#1798
IoT.js-DCO-1.0-Signed-off-by: Philippe Coval p.coval@samsung.com
@rzr rzr force-pushed the sandbox/rzr/devel/review/master branch from fe96267 to b9c7d57 Compare November 14, 2018 13:00
rzr added a commit to TizenTeam/iotjs that referenced this pull request Nov 14, 2018
@rzr
debugger: Disable code if not desired
b9c7d57 
This part is not as critical for TizenRT,
but will help to make iotjs smaller.

Thanks-to: Daeyeon Jeong daeyeon.jeong@samsung.com
Relate-to: jerryscript-project#1795
Forwarded: jerryscript-project#1798
IoT.js-DCO-1.0-Signed-off-by: Philippe Coval p.coval@samsung.com
@rzr rzr force-pushed the sandbox/rzr/devel/review/master branch from b9c7d57 to ffb9eec Compare November 14, 2018 14:20
rzr added a commit to TizenTeam/iotjs that referenced this pull request Nov 14, 2018
@rzr
debugger: Disable code if not desired
ffb9eec 
This part is not as critical for TizenRT,
but will help to make iotjs smaller.

Thanks-to: Daeyeon Jeong daeyeon.jeong@samsung.com
Relate-to: jerryscript-project#1795
Forwarded: jerryscript-project#1798
IoT.js-DCO-1.0-Signed-off-by: Philippe Coval p.coval@samsung.com
rzr added a commit to TizenTeam/iotjs that referenced this pull request Nov 15, 2018
@rzr
debugger: Disable code if not desired
654c1f1 
This part is not as critical for TizenRT,
but will help to make iotjs smaller.

Thanks-to: Daeyeon Jeong daeyeon.jeong@samsung.com
Relate-to: jerryscript-project#1795
Forwarded: jerryscript-project#1798
IoT.js-DCO-1.0-Signed-off-by: Philippe Coval p.coval@samsung.com
@rzr
Copy link
Contributor Author

rzr commented Nov 15, 2018

Please 1st merge:
#1795
before:
#1798
Thanks

LaszloLango
LaszloLango requested changes Nov 15, 2018
View reviewed changes
src/modules/iotjs_module_process.c Outdated
@@ -383,10 +390,12 @@ jerry_value_t InitProcess() {
// Set iotjs
SetProcessIotjs(process);
bool wait_source = false;
#ifdef JERRY_DEBUGGER
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO, this way would be better:

#ifdef JERRY_DEBUGGER
  bool wait_source = false;
  if (iotjs_environment_config(iotjs_environment_get())->debugger != NULL) {
    wait_source = iotjs_environment_config(iotjs_environment_get())
                      ->debugger->wait_source;
  }

  if (!wait_source) {
    SetProcessArgv(process);
  }
#else
  SetProcessArgv(process);
#endif

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isnt compiler figuring it out itself and strip that unused var ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and this same var is used just after so I prefer to declare it false that duplicating the 2 lines

I assume compiler is smart enough to generate the same binary.

@daeyeon
Copy link
Member

daeyeon commented Nov 15, 2018

Please squash the commits into one.

@rzr rzr force-pushed the sandbox/rzr/devel/review/master branch from ffb9eec to 180a9e3 Compare November 15, 2018 13:48
rzr added a commit to TizenTeam/iotjs that referenced this pull request Nov 15, 2018
@rzr
tizenrt: Disable debugger code if not enabled
180a9e3 
Observed build issue on TizenRT (2.0+):

    iotjs.c:58: undefined reference to `jerryx_debugger_tcp_create'
    iotjs.c:59: undefined reference to `jerryx_debugger_ws_create'
    iotjs.c:58: undefined reference to `jerryx_debugger_after_connect'

The whole part is disabled,
even if only jerry-ext functions are not linked.

This should not cause any issue on earlier versions of TizenRT

debugger: Disable code if not desired

This part is not as critical for TizenRT,
but will help to make iotjs smaller.

Thanks-to: Daeyeon Jeong daeyeon.jeong@samsung.com
Relate-to: jerryscript-project#1795
Forwarded: jerryscript-project#1798
IoT.js-DCO-1.0-Signed-off-by: Philippe Coval p.coval@samsung.com
@rzr rzr force-pushed the sandbox/rzr/devel/review/master branch from 180a9e3 to 3b72964 Compare November 15, 2018 14:16
rzr added a commit to TizenTeam/iotjs that referenced this pull request Nov 15, 2018
@rzr
tizenrt: Disable debugger code if not enabled
3b72964 
Observed build issue on TizenRT (2.0+):

    iotjs.c:58: undefined reference to `jerryx_debugger_tcp_create'
    iotjs.c:59: undefined reference to `jerryx_debugger_ws_create'
    iotjs.c:58: undefined reference to `jerryx_debugger_after_connect'

The whole part is disabled,
even if only jerry-ext functions are not linked.

This should not cause any issue on earlier versions of TizenRT

debugger: Disable code if not desired

This part is not as critical for TizenRT,
but will help to make iotjs smaller.

Thanks-to: Daeyeon Jeong daeyeon.jeong@samsung.com
Relate-to: jerryscript-project#1795
Forwarded: jerryscript-project#1798
IoT.js-DCO-1.0-Signed-off-by: Philippe Coval p.coval@samsung.com
@rzr
tizenrt: Disable debugger code if not enabled
f5f3362 
Observed build issue on TizenRT (2.0+):

    iotjs.c:58: undefined reference to `jerryx_debugger_tcp_create'
    iotjs.c:59: undefined reference to `jerryx_debugger_ws_create'
    iotjs.c:58: undefined reference to `jerryx_debugger_after_connect'

The whole part is disabled,
even if only jerry-ext functions are not linked.

This should not cause any issue on earlier versions of TizenRT

debugger: Disable code if not desired

This part is not as critical for TizenRT,
but will help to make iotjs smaller.

Thanks-to: Daeyeon Jeong daeyeon.jeong@samsung.com
Relate-to: jerryscript-project#1795
Forwarded: jerryscript-project#1798
IoT.js-DCO-1.0-Signed-off-by: Philippe Coval p.coval@samsung.com
@rzr rzr force-pushed the sandbox/rzr/devel/review/master branch from 3b72964 to f5f3362 Compare November 15, 2018 14:56
LaszloLango
LaszloLango approved these changes Nov 16, 2018
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

daeyeon
daeyeon approved these changes Nov 16, 2018
View reviewed changes
Copy link
Member

@daeyeon daeyeon left a comment

Choose a reason for hiding this comment

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

LGTM

@rzr rzr mentioned this pull request Nov 20, 2018
Closed
@zherczeg zherczeg merged commit f7adb75 into jerryscript-project:master Nov 20, 2018
@LaszloLango LaszloLango mentioned this pull request Nov 26, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daeyeon daeyeon daeyeon approved these changes

@LaszloLango LaszloLango LaszloLango 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
@rzr @daeyeon @LaszloLango @zherczeg