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.

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

build: reduces unused js2c.py code #7465

Closed
wants to merge 1 commit into from
Closed

build: reduces unused js2c.py code #7465

wants to merge 1 commit into from

Conversation

eljefedelrodeodeljefe
Copy link
Contributor

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

build

Description of change

Just for housekeeping and less confusion, we could get rid of unused code in js2c.py. I didn't see any use of those parts at least and can originate it back to commit 63a9cd3

I dug into this part of the build process and wanted to rewrite js2c.py in JS, which is fairly trivial now, as opposed to 2009. But those parts are just not necessary - pending CI.

likely those parts originate from the single commit 63a9cd3 and have never been
used. This cleans up some of the implementation to have less confusions for devs
visiting this file.
@mscdex mscdex added build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory. labels Jun 28, 2016
@eljefedelrodeodeljefe
Copy link
Contributor Author

@eljefedelrodeodeljefe
Copy link
Contributor Author

CI is green.

@indutny
Copy link
Member

indutny commented Jun 29, 2016

LGTM

@indutny
Copy link
Member

indutny commented Jun 29, 2016

cc @bnoordhuis

@bnoordhuis
Copy link
Member

LGTM but s/reduces/reduce/ (or 'remove' or 'delete') and s/likely/Likely/ in the commit log.

@Fishrock123
Copy link
Contributor

LGTM if it works (with @bnoordhuis's nits). Also, it looks like the commit message may have long line but I may be wrong.

output.write(HEADER_TEMPLATE % {
'builtin_count': len(ids) + len(delay_ids),
'delay_count': len(delay_ids),
'source_lines': "\n".join(source_lines_empty),
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the declaration and accumulation of source_lines_empty as well. It is not used anywhere else.

@bnoordhuis
Copy link
Member

I don't want to steal your thunder but I was going through my open pull requests and I realized I made pretty much the same changes in #5458.

@eljefedelrodeodeljefe
Copy link
Contributor Author

Fine for me. :) closing in favour for #5458

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants