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

Refactor string utils for better consistency #608

Closed
wants to merge 2 commits into from
Closed

Refactor string utils for better consistency #608

wants to merge 2 commits into from

Conversation

MaxGraey
Copy link
Member

Moving parseFloat/parseInt/parseI32/64 from std/string to std/util/string

@dcodeIO
Copy link
Member

dcodeIO commented May 27, 2019

Hmm, initial idea here was that the parseXY functions are actually global (JS) APIs and not really utility, so live in the top level file, while the underlying implementations live in util. Not sure.

@MaxGraey
Copy link
Member Author

In current PR I'm proposing put this in utils and reexport to std/string. My thoughts are better all implementations be in one place instead parse<T> in util/string, rest in std/string.

@dcodeIO
Copy link
Member

dcodeIO commented May 27, 2019

So, the problem seems to be that the implementation of `parseFloat´ is at top-level, while the implementations for integers are in util, I guess? What if we keep the integers, and refactor just parseFloat (maybe becoming generic) to use a similar design?

@MaxGraey
Copy link
Member Author

MaxGraey commented May 27, 2019

Or better move parseInt/parseFloat to buildins.ts. And remove parseI32/parseI64 because we have I32.parseInt/I64.paseInt. WDYT?

@dcodeIO
Copy link
Member

dcodeIO commented May 27, 2019

Aren't really builtins, but one could argue that isNaN and similar should be moved to number. Removing parseI32/I64 sounds good!

@MaxGraey
Copy link
Member Author

I also renamed internal parseFloat to strtod

@@ -331,23 +345,23 @@ export abstract class F64 {
static readonly NaN: f64 = NaN;

static isNaN(value: f64): bool {
return builtin_isNaN<f64>(value);
return isNaN<f64>(value);
Copy link
Member

Choose a reason for hiding this comment

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

Looks like it will conflict if we ever align the resolver with JS to also take the current function's name into account.

Copy link
Member Author

Choose a reason for hiding this comment

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

What if I declare it as function internal_isNaN without export and in the end of file export as:

export { internal_isNaN as isNaN, internal_isFinite as isFinite }

?

}

static isFinite(value: f64): bool {
return builtin_isFinite<f64>(value);
return isFinite<f64>(value);
Copy link
Member

Choose a reason for hiding this comment

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

Same. Not sure how TS handles this if general signatures differ, like one being generic and the other not.

@dcodeIO dcodeIO closed this in 4680b53 Jun 6, 2019
@MaxGraey MaxGraey deleted the refactor-parse-numbers branch June 6, 2019 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants