Skip to content

assert: remove code that is never reached - #8132

Closed
Trott wants to merge 2 commits into
nodejs:masterfrom
Trott:truncatetruncate
Closed

Trott wants to merge 2 commits into
nodejs:masterfrom
Trott:truncatetruncate

Conversation

@Trott

@Trott Trott commented Aug 16, 2016

Copy link
Copy Markdown
Member
Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

assert

Description of change

The internal function truncate() is only called with the first
argument being the output of util.inspect(). util.inspect() calls
its own internal formatValue() which is guaranteed to return a string.

Therefore, we can remove the check in truncate() that the first
argument is a string as well as code to handle the case where it is not
a string.

The internal function `truncate()` is only called with the first
argument being the output of `util.inspect()`. `util.inspect()` calls
its own internal `formatValue()` which is guaranteed to return a string.

Therefore, we can remove the check in `truncate()` that the first
argument is a string as well as code to handle the case where it is not
a string.
@Trott Trott added the assert Issues and PRs related to the assert subsystem. label Aug 16, 2016
@Trott

Trott commented Aug 17, 2016 •

Copy link
Copy Markdown
Member Author

@Trott

Trott commented Aug 17, 2016

Copy link
Copy Markdown
Member Author

There appears to be some Ci infrastructure issues right now...will try again later, but the results look fine on the hosts that didn't have build issues.

@targos

targos commented Aug 17, 2016

Copy link
Copy Markdown
Member

LGTM

Comment thread lib/assert.js Outdated
} else {
return s;
}
return s.length < n ? s : s.slice(0, n);

ghost Aug 17, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not sure. My guess is it was done this way as a micro-optimization for the common case. Seems like just calling s.slice(0,n) as you suggest would be better.

@estliberitas

ghost commented Aug 17, 2016

Copy link
Copy Markdown
Contributor

LGTM

@Trott

ghost commented Aug 17, 2016

Copy link
Copy Markdown
Member Author

@jasnell

ghost commented Aug 17, 2016

Copy link
Copy Markdown
Member

LGTM

1 similar comment
@cjihrig

ghost commented Aug 17, 2016

Copy link
Copy Markdown
Contributor

LGTM

@Trott

ghost commented Aug 18, 2016

Copy link
Copy Markdown
Member Author

Landed in 2e59cce

@Trott Trott closed this Aug 18, 2016
@MylesBorins MylesBorins mentioned this pull request Oct 26, 2016
@Trott
Trott deleted the truncatetruncate branch January 13, 2022 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

assert Issues and PRs related to the assert subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants