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

Project structure #6

Merged
merged 4 commits into from
May 9, 2018
Merged

Project structure #6

merged 4 commits into from
May 9, 2018

Conversation

i4ki
Copy link
Contributor

@i4ki i4ki commented May 6, 2018

Basic code layout

Closes #4

i4ki added 4 commits May 6, 2018 16:23
Signed-off-by: i4k <tiago4orion@gmail.com>
Signed-off-by: i4k <tiago4orion@gmail.com>
Signed-off-by: i4k <tiago4orion@gmail.com>
Signed-off-by: i4k <tiago4orion@gmail.com>
@i4ki i4ki added the enhancement New feature or request label May 6, 2018
@i4ki i4ki requested a review from katcipis May 6, 2018 20:15
@i4ki i4ki added this to the basic working interpreter milestone May 6, 2018
@i4ki i4ki mentioned this pull request May 7, 2018
5 tasks
Copy link
Collaborator

@katcipis katcipis left a comment

Choose a reason for hiding this comment

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

Just some stupid questions and stuff =D


type (
// Node type
Type int
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the package was "node" then node.Type would suffice, but ast.Type is clear enough ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NodeType?

)

// Parse input source into an AST representation.
func Parse(code string) (*ast.AST, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm if JS code is already utf16 then this function should receive a []uint16 no? I may be missing something but strings on Go are always utf8, so this is already encoded on utf8 no ? It could also be a []byte...but a string feels odd to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

JS source could be in any encoding but needs to be converted to UTF-16 internally.

ECMAScript source text is assumed to be a sequence of 16-bit code units for the purposes of this specification. Such a source text may include sequences of 16-bit code units that are not valid UTF-16 character encodings. If an actual source text is encoded in a form other than 16-bit code units it must be processed as if it was first convert to UTF-16.
https://es5.github.io/#x6

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Giving a UTF-8 API (for Eval input) is handy for testing and embed into Go programs. Otherwise clients would have to convert to UTF-16 just to execute something, but enforcing this is not required in the spec.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But in the wild...JS code is delivered as utf8 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By delivered you mean 'developed' ?
I think yes, nowadays everyone agrees that setting the editor to use UTF-8 by default is a good practice. Several editors already uses utf-8 by default independently of the language. But people using other encodings will require a conversion before using (or we deliver API to every encoding).

The spec only says that the native text processing model is UTF-16 code units. That doesn't specify what byte-encoding is used to convert a source file to those units.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found an example of v8 library usage:
https://v8.paulfryzel.com/docs/master/process_8cc-example.html

Take a look in the ReadFile function, that loads the file as UTF-8 and then call String::NewFromUTF8(...) to use in their APIs (they expect a v8::String that is UTF-16):

MaybeLocal<String> result = String::NewFromUtf8(
      isolate, chars.get(), NewStringType::kNormal, static_cast<int>(size));
  return result;

Now search in the file for "NewFromUTF8" and you will see that their APIs are all UTF-16... but then they need to make conversion to everything (including the literal strings) because the source file is UTF-8.

Local<String> process_name =
      String::NewFromUtf8(GetIsolate(), "Process", NewStringType::kNormal)
          .ToLocalChecked();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You feel so, I can change the API of Parse function to:

func Parse(code utf16.Str) (*ast.AST, error)

but then create another function to process from UTF8:

func ParseUTF8(code string) (*ast.AST, error)

But I bet we will never use the UTF-16 API.
Your call.

Copy link
Collaborator

Choose a reason for hiding this comment

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

By delivered I meant when I get a js code on the browser in which encoding it will come (this whole utf16 thing is pretty confusing). Since the language is utf16 it would made sense to think that js code is always stored and transported as utf16...since conversions will be required all the time if not... but if it is actually stored and transported as utf8 then it makes sense that the API is utf8 (although nothings makes sense with JS).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

screenshot from 2018-05-07 19-55-39
Content-Type: application/javascript; charset=utf-8
If not specified, default is utf-8 also. See in the RFC below:
https://tools.ietf.org/html/rfc4329#section-4.2

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the clarification =D

}

func (t Type) String() string {
return names[t]
Copy link
Collaborator

Choose a reason for hiding this comment

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

if we add a new type and forget adding it to the map we want a panic ? (could also return a string like "invalid type" etc)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the right thing to do is panic. I'll change in the other PR (#9).
Thanks

@i4ki i4ki merged commit 849bf10 into master May 9, 2018
@i4ki i4ki deleted the project-structure branch May 9, 2018 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants