[wasm][asm.js] Adding custom asm.js lexer.

src/asmjs/asm-lexer.h

+// Copyright 2017 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef V8_ASMJS_ASM_LEXER_H_
+#define V8_ASMJS_ASM_LEXER_H_
+
+#include <unordered_map>
+
+#include "src/asmjs/asm-names.h"
+#include "src/objects-inl.h"
+#include "src/objects.h"

[marja, 2017/03/14 11:11:47]

Nit: You shouldn't need objects.h and objects-inl.h, forward declarations should
be enough. You need handles.h though.

[bradn, 2017/03/15 07:53:04]

Done.

+#include "src/parsing/scanner.h"

[marja, 2017/03/14 11:11:47]

Can you split out the stream from scanner.h so that you don't need to include
scanner.h?

[vogelheim, 2017/03/14 13:36:37]

Forward declare v8::internal::Utf16CharacterStream ?

[bradn, 2017/03/15 07:53:03]

Done.

[bradn, 2017/03/15 07:53:04]

Done.

+
+namespace v8 {
+namespace internal {
+
+class AsmJsLexer {
+ public:
+  typedef intptr_t token_t;

[vogelheim, 2017/03/14 13:36:38]

Why intptr_t? Is it intentional that the token range is platform dependent?
(32/64b)

[bradn, 2017/03/15 07:53:04]

Not particularly, other than that the string table size could in principle push
things up to container size. Should never be that large in practice. Changed to
int32_t and added overflow checks.

+
+  explicit AsmJsLexer(Isolate* isolate, Handle<Script> script, int start,

[vogelheim, 2017/03/14 13:36:38]

nitpick: Drop explicit with multi-arg constructor.

[bradn, 2017/03/15 07:53:04]

Done.

+                      int end);
+  token_t Token() const { return token_; }
+  const std::string& name() const { return name_; }

[vogelheim, 2017/03/14 13:36:37]

name() / name_ appears to be the current token buffer. If so, it should have a
different name.

[vogelheim, 2017/03/14 13:36:37]

What is the intended use of name()?

I take it the point of this lexer implementation was that all identifiers are
assigned to unique token value so that the parser won't have to consider them.

[bradn, 2017/03/15 07:53:04]

Divided up its uses, renamed the public part to IdentifierString()

[bradn, 2017/03/15 07:53:04]

So the vast majority of places just the token id can be used.
However in a few places (exports, imports), the string of the name is used to
decide names in the generated code. But you know there from context you'll need
it as you parse, so no need to carry it around with each token in general.
I support I could make lookup more efficient, but figured just keeping the last
one works too.

+  void SetLocalScope(bool local) { local_ = local; }

[vogelheim, 2017/03/14 13:36:38]

What does this do? (I mean, what's a local scope, and how does lexing for a
local scope differ?)

[bradn, 2017/03/15 07:53:04]

Renamed, commented, and clarified.

+  void Next();
+  void Rewind();
+  void ResetLocals();
+  const char* Name(token_t token) const;
+  int position() const;

[Karl, 2017/03/14 18:00:47]

Why is this method lower case when other methods start with a capital?

[bradn, 2017/03/15 07:53:04]

Changed.

+  void Seek(int pos);
+
+  bool IsLocal() const { return IsLocal(Token()); }
+  bool IsGlobal() const { return IsGlobal(Token()); }
+  static bool IsLocal(token_t token) { return token <= kLocalsStart; }
+  static bool IsGlobal(token_t token) { return token >= kGlobalsStart; }
+  static size_t LocalIndex(token_t token) {
+    DCHECK(IsLocal(token));
+    return -(token - kLocalsStart);
+  }
+  static size_t GlobalIndex(token_t token) {
+    DCHECK(IsGlobal(token));
+    return token - kGlobalsStart;
+  }
+  bool preceeded_by_newline() const { return preceeded_by_newline_; }
+
+  bool IsUnsigned() const { return Token() == kUnsigned; }
+  bool IsDouble() const { return Token() == kDouble; }
+
+  double AsDouble() const { return double_value_; }
+  uint64_t AsUnsigned() const { return unsigned_value_; }
+
+  enum {

[vogelheim, 2017/03/14 13:36:37]

This enum has a lot of implied structure which is relied on in other methods,
but isn't document anywhere. You may want to add a comment explaining the value
ranges and their meaning/properties.

[bradn, 2017/03/15 07:53:04]

Done.

+    kLocalsStart = -10000,
+#define V(name, _junk1, _junk2, _junk3) kToken_##name,
+    STDLIB_MATH_FUNCTION_LIST(V) STDLIB_ARRAY_TYPE_LIST(V)
+#undef V
+#define V(name) kToken_##name,
+        STDLIB_OTHER_LIST(V) STDLIB_MATH_VALUE_LIST(V) KEYWORD_NAME_LIST(V)
+#undef V
+#define V(rawname, name) kToken_##name,
+            LONG_SYMBOL_NAME_LIST(V)

[vogelheim, 2017/03/14 13:36:38]

formatting nitpick: The indent is funky. Slightly rewrite (I think empty lines
will yield a different result) or consider guiding clang-format w/ '//
clang-format on|off' comments.

[bradn, 2017/03/15 07:53:04]

Done.

+#undef V
+
+                kEndOfInput = -1,
+    kParseError = -2,
+    kUnsigned = -3,
+    kDouble = -4,
+    kGlobalsStart = 256,
+  };
+
+ private:
+  Handle<Script> script_;
+  Handle<String> source_;
+  std::unique_ptr<Utf16CharacterStream> stream_;
+  token_t token_;
+  token_t last_token_;

[vogelheim, 2017/03/14 13:36:37]

last_token_ -> preceding_token_?

(I take it it's the token we processed before the current_token_, rather than
the last token in the stream.)

[bradn, 2017/03/15 07:53:04]

Yes.
Renamed.

+  token_t next_token_;
+  bool rewind_;
+  std::string name_;
+  bool local_;
+  std::unordered_map<std::string, token_t> local_names_;
+  std::unordered_map<std::string, token_t> global_names_;
+  std::unordered_map<std::string, token_t> property_names_;
+  int global_count_;
+  double double_value_;
+  uint64_t unsigned_value_;
+  bool preceeded_by_newline_;
+};
+
+}  // namespace internal
+}  // namespace v8
+#endif