GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — src: remove duplicated code in `GenerateSingleExecutableBlob()` by pluris · Pull Request #49119 · nodejs/node

Conversation

@pluris

@pluris pluris commented Aug 12, 2023

Copy link
Copy Markdown
Contributor

The removed optional_code_cache variable is already declared with the same name in the line below.
In my opinion, it was judged to be unnecessary redundant code and was removed.

node/src/node_sea.cc

Lines 489 to 494 in 3224527

std::optional<std::string> optional_code_cache =
GenerateCodeCache(config.main_path, main_script);
if (!optional_code_cache.has_value()) {
FPrintF(stderr, "Cannot generate V8 code cache\n");
return ExitCode::kGenericUserError;
}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss and illusi0n7 reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
pluris
src: remove duplicated code in GenerateSingleExecutableBlob()
844339d
@nodejs-github-bot

nodejs-github-bot commented Aug 12, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/single-executable
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. single-executable Issues and PRs related to single-executable applications. labels Aug 12, 2023
lpinca
lpinca approved these changes Aug 12, 2023
View reviewed changes
deokjinkim
deokjinkim reviewed Aug 12, 2023
View reviewed changes
Comment thread src/node_sea.cc
Comment on lines -499 to -504
std::optional<std::string> optional_code_cache =
GenerateCodeCache(config.main_path, main_script);
if (!optional_code_cache.has_value()) {
FPrintF(stderr, "Cannot generate V8 code cache\n");
return ExitCode::kGenericUserError;
}

@deokjinkim deokjinkim Aug 12, 2023

Copy link
Copy Markdown
Contributor

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.

IMO, these lines(499-504) are only needed if flag for code cache is enabled. So how about removing above declaration(489-494) instead of this? Plus, declaration of code_cache variable(497) can be moved into if statement.

@RaisinTen PTAL when you are available.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
RaisinTen and pluris reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

@pluris pluris Aug 13, 2023

Copy link
Copy Markdown
Contributor Author

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.

@deokjinkim
Thank you for your good comments.
You are right. It seems like a better idea. I reflected what you said.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@pluris
pluris requested a review from deokjinkim August 13, 2023 06:18
pluris
fixup! src: remove duplicated code in GenerateSingleExecutableBlob()
0f27c01
@pluris
pluris force-pushed the fix/remove_unnessary_code branch from 4026cf4 to 0f27c01 Compare August 13, 2023 06:19
deokjinkim
deokjinkim approved these changes Aug 13, 2023
View reviewed changes
@deokjinkim deokjinkim added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 13, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 13, 2023
@nodejs-github-bot

nodejs-github-bot commented Aug 13, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53274/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tony-go
tony-go approved these changes Aug 14, 2023
View reviewed changes
RaisinTen
RaisinTen approved these changes Aug 14, 2023
View reviewed changes

@RaisinTen RaisinTen left a comment

Copy link
Copy Markdown
Member

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.

LGTM, thanks for catching this

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@RaisinTen RaisinTen added author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue PRs queued for automated landing through the Commit Queue. commit-queue-squash PRs the Commit Queue should land as one squashed commit. labels Aug 14, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Aug 14, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 6476c99 into nodejs:main Aug 14, 2023
@nodejs-github-bot

nodejs-github-bot commented Aug 14, 2023

Copy link
Copy Markdown
Collaborator

Landed in 6476c99
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pluris
pluris deleted the fix/remove_unnessary_code branch August 15, 2023 05:18
RafaelGSS pushed a commit that referenced this pull request Aug 15, 2023
Jungku Lee @RafaelGSS
src: remove duplicated code in GenerateSingleExecutableBlob()
11b7617 
PR-URL: #49119
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Deokjin Kim <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
@UlisesGascon UlisesGascon mentioned this pull request Aug 15, 2023
v20.6.0 proposal #49185
Merged
RafaelGSS pushed a commit to RafaelGSS/node that referenced this pull request Aug 15, 2023
Jungku Lee @RafaelGSS
src: remove duplicated code in GenerateSingleExecutableBlob()
eba3a40 
PR-URL: nodejs#49119
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Deokjin Kim <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Aug 16, 2023
Jungku Lee @RafaelGSS
src: remove duplicated code in GenerateSingleExecutableBlob()
bceaec3 
PR-URL: #49119
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Deokjin Kim <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Aug 17, 2023
Jungku Lee @RafaelGSS
src: remove duplicated code in GenerateSingleExecutableBlob()
8d6948f 
PR-URL: #49119
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Deokjin Kim <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lpinca lpinca lpinca approved these changes
@deokjinkim deokjinkim deokjinkim approved these changes
@RaisinTen RaisinTen RaisinTen approved these changes
+1 more reviewer
@tony-go tony-go tony-go approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue-squash PRs the Commit Queue should land as one squashed commit. needs-ci PRs that need a full CI run. single-executable Issues and PRs related to single-executable applications.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@pluris @nodejs-github-bot @lpinca @deokjinkim @tony-go @RaisinTen
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.