[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)
FWIW, Niko is looking into simplifying the rewriting mechanism as part of his patch now. https://codereview.chromium.org/1309813007/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)
On 2015/12/04 15:31:49, caitp wrote:
On 2015/12/04 15:16:33, rossberg wrote:
> On 2015/12/04 14:35:32, caitp wrote:
> > On 2015/12/04 14:34:43, caitp wrote:
> > > On 2015/12/04 14:32:33, rossberg wrote:
> > > > On 2015/12/04 14:26:09, caitp wrote:
> > > > > On 2015/12/04 14:24:21, rossberg wrote:
> > > > > > The main thing I'm confused about is why we need the rewriting
queue.
> > What
> > > > > > prevents you from invoking the rewriting right at the time
where
you
> > > > currently
> > > > > > just queue it up?
> > > > >
> > > > > The main thing is the ambiguity `({ x } = { x: 1 }) => x` <<
where
the
> > > meaning
> > > > > changes depending on whether `=>` is found or not. Binding
patterns
are
> > > > > rewritten differently, so by necessity the rewriting is deferred
until
> > it's
> > > > > known for sure what is being rewritten
> > > >
> > > > But is anything ever removed from this queue without being
rewritten?
I
> > can't
> > > > find that happening anywhere, so isn't it always performed
unconditionally
> > > > anyway, just delayed?
> > >
> > > Hmm, that's a good point, right now there might be some redundant
rewriting
> > > happening. I'm not sure it should be removed from the queue because
it
can't
> > be
> > > easily, but it can be marked as rewritten to prevent redundant
rewriting
> >
> > --- but, just to be clear, that redundant rewriting doesn't mean that
it's
> able
> > to eagerly rewrite, it just means that the redundant rewriting is
sort of
> > discarded, because the RewritableAssignmentExpression itself is
discarded
>
> I see. The deeper reason I'm asking is because Niko is now implementing
the
> rewriting for array literals with spread, which has a similar problem.
The
way
I
> thought this would work is that we introduce something dual to the
pattern
> rewriter, i.e., an expression visitor that is invoked when an ambiguous
> cover-expression/pattern is resolved to be an expression. That would
then
walk
> and rewrite the necessary expressions inside. (In most cases that would
be a
> no-op, so it's probably worth introducing a bit in the expression
classifier
to
> indicate the necessity for rewriting.)
>
> You already have that kind of visitor, it seems, so I wonder why it
isn't
> possible to invoke it directly at the resolution points.
It may be possible, I've been looking at this CL for too long now, almost
4
months. Should it wait to see if I can find a better way to deal with the
cover
grammar?
I don't want to block this. So perhaps you land it and then look if it can
be
simplified in a follow up?
https://codereview.chromium.org/1309813007/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)
On 2015/12/04 14:35:32, caitp wrote:
On 2015/12/04 14:34:43, caitp wrote:
> On 2015/12/04 14:32:33, rossberg wrote:
> > On 2015/12/04 14:26:09, caitp wrote:
> > > On 2015/12/04 14:24:21, rossberg wrote:
> > > > The main thing I'm confused about is why we need the rewriting
queue.
What
> > > > prevents you from invoking the rewriting right at the time where
you
> > currently
> > > > just queue it up?
> > >
> > > The main thing is the ambiguity `({ x } = { x: 1 }) => x` << where
the
> meaning
> > > changes depending on whether `=>` is found or not. Binding patterns
are
> > > rewritten differently, so by necessity the rewriting is deferred
until
it's
> > > known for sure what is being rewritten
> >
> > But is anything ever removed from this queue without being rewritten?
I
can't
> > find that happening anywhere, so isn't it always performed
unconditionally
> > anyway, just delayed?
>
> Hmm, that's a good point, right now there might be some redundant
rewriting
> happening. I'm not sure it should be removed from the queue because it
can't
be
> easily, but it can be marked as rewritten to prevent redundant rewriting
--- but, just to be clear, that redundant rewriting doesn't mean that it's
able
to eagerly rewrite, it just means that the redundant rewriting is sort of
discarded, because the RewritableAssignmentExpression itself is discarded
I see. The deeper reason I'm asking is because Niko is now implementing the
rewriting for array literals with spread, which has a similar problem. The
way I
thought this would work is that we introduce something dual to the pattern
rewriter, i.e., an expression visitor that is invoked when an ambiguous
cover-expression/pattern is resolved to be an expression. That would then
walk
and rewrite the necessary expressions inside. (In most cases that would be a
no-op, so it's probably worth introducing a bit in the expression
classifier to
indicate the necessity for rewriting.)
You already have that kind of visitor, it seems, so I wonder why it isn't
possible to invoke it directly at the resolution points.
https://codereview.chromium.org/1309813007/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)
On 2015/12/04 14:26:09, caitp wrote:
On 2015/12/04 14:24:21, rossberg wrote:
> The main thing I'm confused about is why we need the rewriting queue.
What
> prevents you from invoking the rewriting right at the time where you
currently
> just queue it up?
The main thing is the ambiguity `({ x } = { x: 1 }) => x` << where the
meaning
changes depending on whether `=>` is found or not. Binding patterns are
rewritten differently, so by necessity the rewriting is deferred until
it's
known for sure what is being rewritten
But is anything ever removed from this queue without being rewritten? I
can't
find that happening anywhere, so isn't it always performed unconditionally
anyway, just delayed?
https://codereview.chromium.org/1309813007/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [es6] parse destructuring assignment (issue 1309813007 by caitpotte...@gmail.com)
The main thing I'm confused about is why we need the rewriting queue. What prevents you from invoking the rewriting right at the time where you currently just queue it up? https://codereview.chromium.org/1309813007/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
Re: [v8-dev] Code structure bikeshed: Where should spec reference implementations live?
Some quick thoughts. Option (4) is a non-starter. Section numbers are not stable across spec releases. Option (3) doesn't sound bad, although it's not quite clear what the criteria for putting something into objects vs runtime vs the new dir would be. If we introduced a third category, not only stuff from runtime but also significant parts of the logic in objects.cc should naturally move there. Sounds like a lot of work with unclear benefit. Option (1) seems like the most adequate for now. IMHO, thinking about a new directory structure is putting the cart before the horse at this point. It only is interesting as part of a broader strategy for splitting up objects.h/cc. I don't think we currently have any plausible plan for that. /Andreas On 25 September 2015 at 11:58, Jakob Kummerow wrote: > As we have discussed at various occasions recently, we generally want to > move in the direction of having C++ implementations of spec-defined > behavior. That raises the question of where this code should live. > > As an example of the kind of code we're talking about, consider > https://codereview.chromium.org/1368753003/diff/1/src/runtime/runtime-object.cc > *(don't > panic, runtime-object.cc is not the intended final place for this code to > live -- the very purpose of this thread is to figure out a better place.)*, > but there are also other, existing examples (like various ToXXX conversion > functions, a bunch of things spread across runtime-*.cc, the JS > implementations littered with runtime calls that we want to replace, ...). > > Options I can think of: > > (1) Put everything into objects.cc. > + Makes a lot of sense for things like DefineOwnProperty_Array, which > could just be a static function JSArray::DefineOwnProperty. > + Is an easy approach in the sense of being consistent with existing code > structure (is that a good thing?) > − It's not clear how this approach maps to non-HeapObjects like the new > class PropertyDescriptor > − I like having some distinction between high-level spec-defined > operations like "DefineOwnProperty" and low-level V8 implementation details > like MigrateToMap -- installing both on the same class JSObject feels like > a recipe for confusion. > − objects.h/.cc are too big as it is, IMHO (of course this point is moot > if/when we split it up) > > (2) Put everything in runtime-*.cc > + Works, and there's plenty of precedent. > − AFAIK we have pretty wide consensus that that's not what we want. > − A concrete technical drawback is a lack of callability from other places. > > (3) Create a new directory, put everything there. > + All reference implementations would be in one place > + Can use individual files for further grouping if desired. Is that > desired? What file structure would be good? > + Personally I think we need more separation of things anyway, this is a > step in that direction > • next question: how to call that directory? src/spec/? src/es6/? > /src/blue/? (blue sheds are nice) > − For some things it might be unclear where to put them; our > "abstractions" are (necessarily?) leaky > − New thing to get used to; inconsistency while it's a work in progress > > (4) Organize by spec chapter, e.g. put OrdinaryDefineOwnProperty into > src/es2015/ch9/9.1.cc or somesuch > + If applied consistently, makes it easy to find things that are already > implemented, which avoids duplication > − the resulting grouping may or may not make sense (it's up to the spec) > − ugly > > Personally I'm leaning towards some variant of (3), but I'm open to being > convinced otherwise. (1) sounds like a temporary solution to me; why not go > for a longer-term plan right away? > > Thoughts? Other ideas? Indifference? > > -- > -- > v8-dev mailing list > v8-dev@googlegroups.com > http://groups.google.com/group/v8-dev > --- > You received this message because you are subscribed to the Google Groups > "v8-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to v8-dev+unsubscr...@googlegroups.com. > For more options, visit https://groups.google.com/d/optout. > -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Always lazy compile arrow functions (issue 1317033005 by ad...@chromium.org)
https://codereview.chromium.org/1317033005/diff/20001/src/preparser.h
File src/preparser.h (right):
https://codereview.chromium.org/1317033005/diff/20001/src/preparser.h#newcode3288
src/preparser.h:3288:
!IsArrowFunction(result->AsFunctionLiteral()->kind())) {
On 2015/09/19 21:34:36, caitp wrote:
The only time you _need_ to force re-parsing of arrows is when the
parameter
list contains expressions (eg an initializer), so we could probably
add an extra
condition to escape lazy parsing here. But, it might not matter that
much
anyway, so.
That's a good point. It might make sense to avoid enforcing lazy parsing
for arrow functions with simple parameters. I expect arrow IIFE to
become a not too-uncommon use case.
https://codereview.chromium.org/1317033005/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[v8-dev] Do not permit binding over catch parameters in for-of (issue 1318023004 by ejcar...@chromium.org)
Hm, I don't quite understand this CL. It seems to special-case for-loops, but there are many other constructs that could introduce conflicting bindings. Shouldn't these conflicts be detected in a more central place? Specifically, I think you want to invoke something similar to the existing CheckConflictingDeclarations in the function that parses the `catch` phrase. https://codereview.chromium.org/1318023004/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [es6] re-implement rest parameters via desugaring (issue 1272673003 by caitpotte...@gmail.com)
https://codereview.chromium.org/1272673003/diff/230001/src/parser.h File src/parser.h (right): https://codereview.chromium.org/1272673003/diff/230001/src/parser.h#newcode1324 src/parser.h:1324: !is_rest && pattern->IsVariableProxy() && initializer == nullptr; On 2015/09/02 20:11:31, caitp wrote: This makes sure the empty string is used instead of the variable name, so that the variable doesn't get declared twice. This seems to fix the bug in the tests. I'm not sure if this code should be here, or maybe live somewhere else, but it should be okay either way. Yes, this looks right to me. https://codereview.chromium.org/1272673003/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Bubble up the transitions associated with PreventExtensionsWithTransition (issue 1239803004 by conr...@chromium.org)
Igor, given that Toon is away, can you perhaps have a look at this CL? https://codereview.chromium.org/1239803004/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [es6] Parameter scopes for sloppy eval (issue 1292753007 by rossb...@chromium.org)
On 2015/09/02 12:22:32, ruv wrote: On 2015/08/19 16:10:23, rossberg wrote: > - In the parser, if a function has a sloppy direct eval, introduce an additional varblock scope around each non-simple (desugared) parameter, as required by the spec to contain possible dynamic var bindings. Could you please clarify, does the engine take into account that in strict mode such eval doesn't introduce new variables into the surrounding scope? So, in such case no need to allow possible dynamic var bindings. Yes, strict cannot contain a "sloppy direct eval". (Though do-expressions may be another source of vars in the future, even in strict mode.) https://codereview.chromium.org/1292753007/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [es6] re-implement rest parameters via desugaring (issue 1272673003 by caitpotte...@gmail.com)
Parser changes LGTM, haven't looked at the rest. https://codereview.chromium.org/1272673003/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [strong] Class constructor bodies cannot contain "use strong" directive (issue 1314203002 by conr...@chromium.org)
LGTM modulo comment https://codereview.chromium.org/1314203002/diff/40001/test/mjsunit/strong/function-arity.js File test/mjsunit/strong/function-arity.js (left): https://codereview.chromium.org/1314203002/diff/40001/test/mjsunit/strong/function-arity.js#oldcode220 test/mjsunit/strong/function-arity.js:220: `'use strict'; I don't think you want to delete this test altogether. It would still be good to have test for a strict class deriving from a strong one, though you'll have to write it slightly differently. https://codereview.chromium.org/1314203002/diff/40001/test/mjsunit/strong/function-arity.js#oldcode256 test/mjsunit/strong/function-arity.js:256: `'use strict'; Same here perhaps. https://codereview.chromium.org/1314203002/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] [es6] Implement destructuring for `catch` (issue 1315483004 by rossb...@chromium.org)
Reviewers: adamk,
Message:
Hi Adam, this is as far as I got so far. Please feel free to grab. I just
got it
to compile, but currently breaks some existing stuff. I didn't yet get to
finding out why.
Description:
[es6] Implement destructuring for `catch`
WIP
BUG=v8:811
LOG=N
Please review this at https://codereview.chromium.org/1315483004/
Base URL: https://chromium.googlesource.com/v8/v8.git@master
Affected files (+73, -15 lines):
M src/ast-value-factory.h
M src/parser.cc
M src/preparser.cc
Index: src/ast-value-factory.h
diff --git a/src/ast-value-factory.h b/src/ast-value-factory.h
index
359cac8133d962b3c9695dca3360c904aae83a5e..57b3e313bf6987c9b35045d1b3ff33f653fcbbc3
100644
--- a/src/ast-value-factory.h
+++ b/src/ast-value-factory.h
@@ -249,6 +249,7 @@ class AstValue : public ZoneObject {
F(default, "default")
\
F(done, "done")
\
F(dot, ".")
\
+ F(dot_catch, ".catch")
\
F(dot_for, ".for")
\
F(dot_generator, ".generator")
\
F(dot_generator_object, ".generator_object")
\
Index: src/parser.cc
diff --git a/src/parser.cc b/src/parser.cc
index
9334a33195a1e21f474055863bc28dd47fa2cb93..691f9fb83f43e3905f24e7676d406711cc334201
100644
--- a/src/parser.cc
+++ b/src/parser.cc
@@ -3041,21 +3041,63 @@ TryStatement* Parser::ParseTryStatement(bool* ok) {
Scope* catch_scope = NULL;
Variable* catch_variable = NULL;
Block* catch_block = NULL;
- const AstRawString* name = NULL;
if (tok == Token::CATCH) {
Consume(Token::CATCH);
Expect(Token::LPAREN, CHECK_OK);
catch_scope = NewScope(scope_, CATCH_SCOPE);
catch_scope->set_start_position(scanner()->location().beg_pos);
-name = ParseIdentifier(kDontAllowRestrictedIdentifiers, CHECK_OK);
+
+ExpressionClassifier pattern_classifier;
+Token::Value next = peek();
+Expression* pattern =
+ParsePrimaryExpression(&pattern_classifier, CHECK_OK);
+ValidateBindingPattern(&pattern_classifier, CHECK_OK);
+
+if (!allow_harmony_destructuring() && !pattern->IsVariableProxy()) {
+ ReportUnexpectedToken(next);
+ *ok = false;
+ return NULL;
+}
Expect(Token::RPAREN, CHECK_OK);
-catch_variable = catch_scope->DeclareLocal(name, VAR,
kCreatedInitialized,
- Variable::NORMAL);
-BlockState block_state(&scope_, catch_scope);
-catch_block = ParseBlock(NULL, CHECK_OK);
+if (pattern->IsVariableProxy()) {
+ const AstRawString* name = pattern->AsVariableProxy()->raw_name();
+ catch_variable = catch_scope->DeclareLocal(name, VAR,
kCreatedInitialized,
+ Variable::NORMAL);
+ BlockState block_state(&scope_, catch_scope);
+ catch_block = ParseBlock(NULL, CHECK_OK);
+} else {
+ Variable* temp = scope_->NewTemporary(
+ ast_value_factory()->dot_catch_string());
+ Scope* destruct_scope = NewScope(catch_scope, BLOCK_SCOPE);
+ Block* destruct_block =
+ factory()->NewBlock(nullptr, 1, false, RelocInfo::kNoPosition);
+
+ DeclarationDescriptor descriptor;
+ descriptor.declaration_kind = DeclarationDescriptor::NORMAL;
+ descriptor.parser = this;
+ descriptor.declaration_scope = destruct_scope->DeclarationScope();
+ descriptor.scope = destruct_scope;
+ descriptor.hoist_scope = nullptr;
+ descriptor.mode = LET;
+ descriptor.is_const = false;
+ descriptor.needs_init = true;
+ descriptor.declaration_pos = RelocInfo::kNoPosition;
+ descriptor.initialization_pos = RelocInfo::kNoPosition;
+ descriptor.init_op = Token::INIT_LET;
+ Expression* initial_value = factory()->NewVariableProxy(temp);
+
+ BlockState block_state(&scope_, destruct_scope);
+ DeclarationParsingResult::Declaration decl(
+ pattern, pattern->position(), initial_value);
+ PatternRewriter::DeclareAndInitializeVariables(
+ destruct_block, &descriptor, &decl, nullptr, CHECK_OK);
+
+ catch_block->AddStatement(destruct_block, zone());
+ catch_block = ParseBlock(NULL, CHECK_OK);
+}
catch_scope->set_end_position(scanner()->location().end_pos);
tok = peek();
Index: src/preparser.cc
diff --git a/src/preparser.cc b/src/preparser.cc
index
b1541f616afe133554881e99aec4f71a9f5e45e0..a104678f9fb0f43a9ea6a75d9bf8c0d127536973
100644
--- a/src/preparser.cc
+++ b/src/preparser.cc
@@ -496,15 +496,11 @@ PreParser::Statement
PreParser::ParseVariableDeclarations(
// VariableDeclarations ::
// ('var' | 'const') (Identifier ('=' AssignmentExpression)?)+[',']
//
- // The ES6 Draft Rev3 speci
[v8-dev] Re: Move runtime helper for JSArrayBuffer onto objects. (issue 1305383003 by mstarzin...@chromium.org)
lgtm https://codereview.chromium.org/1305383003/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Move runtime helper for JSWeakCollection onto objects. (issue 1314053003 by mstarzin...@chromium.org)
lgtm https://codereview.chromium.org/1314053003/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [es6] Make harmony_destructuring imply harmony_default_parameters (issue 1317843002 by conr...@chromium.org)
lgtm https://codereview.chromium.org/1317843002/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [es6] Initial steps towards a correct implementation of IsCallable. (issue 1316933002 by bmeu...@chromium.org)
Looks mostly good to me, I just wonder how confident we can be that using
IS_CALLABLE everywhere doesn't allow tons of cases we don't actually handle.
https://codereview.chromium.org/1316933002/diff/1/src/objects-debug.cc
File src/objects-debug.cc (right):
https://codereview.chromium.org/1316933002/diff/1/src/objects-debug.cc#newcode39
src/objects-debug.cc:39: CHECK(!IsCallable());
Do we want to check this condition in more cases?
https://codereview.chromium.org/1316933002/diff/1/src/promise.js
File src/promise.js (right):
https://codereview.chromium.org/1316933002/diff/1/src/promise.js#newcode262
src/promise.js:262: : PromiseIdResolveHandler;
Nit: should fit one line now (same below)
https://codereview.chromium.org/1316933002/diff/1/src/runtime/runtime-classes.cc
File src/runtime/runtime-classes.cc (right):
https://codereview.chromium.org/1316933002/diff/1/src/runtime/runtime-classes.cc#newcode107
src/runtime/runtime-classes.cc:107: } else if
(super_class->IsJSFunction()) {
Shouldn't this be IsCallable as well? It's perfectly legal to inherit
from, say, a function proxy.
https://codereview.chromium.org/1316933002/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Move runtime healper for JSSet and JSMap onto objects. (issue 1312413002 by mstarzin...@chromium.org)
lgtm https://codereview.chromium.org/1312413002/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Ensure hole checks take place in switch statement scopes (issue 1312613003 by little...@chromium.org)
On 2015/08/26 12:07:58, rossberg wrote:
https://codereview.chromium.org/1312613003/diff/60001/src/scopes.h
File src/scopes.h (right):
https://codereview.chromium.org/1312613003/diff/60001/src/scopes.h#newcode232
src/scopes.h:232: void SetNonlinear() { scope_nonlinear_ = true; }
On 2015/08/26 11:30:30, Michael Starzinger wrote:
> Isn't it necessary to serialize this flag into the ScopeInfo? Adding
Andreas
> whose scoping foo is clearly superior to mine.
Indeed, AFAICS this flag needs to be de/serialized in scopeinfos,
otherwise
lazily compiled local functions inside switch blocks will still miss hole
checks. A test for that case would be good, too.
...and likewise in local evals, me thinks.
https://codereview.chromium.org/1312613003/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Ensure hole checks take place in switch statement scopes (issue 1312613003 by little...@chromium.org)
https://codereview.chromium.org/1312613003/diff/60001/src/scopes.h
File src/scopes.h (right):
https://codereview.chromium.org/1312613003/diff/60001/src/scopes.h#newcode232
src/scopes.h:232: void SetNonlinear() { scope_nonlinear_ = true; }
On 2015/08/26 11:30:30, Michael Starzinger wrote:
Isn't it necessary to serialize this flag into the ScopeInfo? Adding
Andreas
whose scoping foo is clearly superior to mine.
Indeed, AFAICS this flag needs to be de/serialized in scopeinfos,
otherwise lazily compiled local functions inside switch blocks will
still miss hole checks. A test for that case would be good, too.
https://codereview.chromium.org/1312613003/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: In generators, "yield" cannot be an arrow formal parameter name (issue 1309523005 by wi...@igalia.com)
lgtm https://codereview.chromium.org/1309523005/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [turbofan] Fix broken dynamic TDZ check for let and const. (issue 1318693002 by mstarzin...@chromium.org)
lgtm https://codereview.chromium.org/1318693002/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Visit AST Property nodes as expressions in AstExpressionVisitor. (issue 1314843002 by bradnel...@google.com)
lgtm https://codereview.chromium.org/1314843002/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [es6] Remaining cases of parameter scopes for sloppy eval (issue 1303013007 by rossb...@chromium.org)
https://codereview.chromium.org/1303013007/diff/1/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/1303013007/diff/1/src/parser.cc#newcode4390 src/parser.cc:4390: CheckConflictingVarDeclarations(param_scope, CHECK_OK); On 2015/08/25 20:18:12, Dan Ehrenberg wrote: Not new in this patch, but I'm wondering why we check this when it's a new declaration scope that's introduced. I don't see how you could come up with a conflict now, but with do-expressions I could imagine it, and that var declaration should just not be visible anywhere after the parameters are evaluated. Indeed, I did this with do-expressions in mind. Seemed "more correct" now, and one thing less to overlook later. :) https://codereview.chromium.org/1303013007/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Parse arrow functions at proper precedence level (issue 1315823002 by wi...@igalia.com)
LGTM
https://codereview.chromium.org/1315823002/diff/20001/src/preparser.h
File src/preparser.h (right):
https://codereview.chromium.org/1315823002/diff/20001/src/preparser.h#newcode2279
src/preparser.h:2279: } else if (allow_harmony_rest_parameters() &&
Check(Token::ELLIPSIS)) {
On 2015/08/25 13:18:18, wingo wrote:
On 2015/08/25 12:55:56, rossberg wrote:
> I wonder, does rest actually need special treatment here? Can't it
simply be
> parsed as spread in the else case below, and later be rewritten?
I think it does need special treatment. All the other places that
parse spread
(array literals, call arguments, comma expressions) parse spread
specially. We
could of course parse it anywhere as a prefix unary op, marking the
production
invalid as an expression, but that is a riskier change if we are
considering
uplifting this to M46. WDYT?
SGTM. Let's leave that as a clean-up for a separate CL.
https://codereview.chromium.org/1315823002/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Parse arrow functions at proper precedence level (issue 1315823002 by wi...@igalia.com)
https://codereview.chromium.org/1315823002/diff/20001/src/interpreter/bytecode-generator.cc
File src/interpreter/bytecode-generator.cc (right):
https://codereview.chromium.org/1315823002/diff/20001/src/interpreter/bytecode-generator.cc#newcode339
src/interpreter/bytecode-generator.cc:339: UNIMPLEMENTED();
Nit: This should probably be UNREACHABLE (same for Spread)
https://codereview.chromium.org/1315823002/diff/20001/src/pattern-rewriter.cc
File src/pattern-rewriter.cc (right):
https://codereview.chromium.org/1315823002/diff/20001/src/pattern-rewriter.cc#newcode367
src/pattern-rewriter.cc:367: // TODO(dslomov): implement.
While you're here, can you change this to UNREACHABLE?
https://codereview.chromium.org/1315823002/diff/20001/src/preparser.h
File src/preparser.h (right):
https://codereview.chromium.org/1315823002/diff/20001/src/preparser.h#newcode2279
src/preparser.h:2279: } else if (allow_harmony_rest_parameters() &&
Check(Token::ELLIPSIS)) {
I wonder, does rest actually need special treatment here? Can't it
simply be parsed as spread in the else case below, and later be
rewritten?
https://codereview.chromium.org/1315823002/diff/20001/src/prettyprinter.cc
File src/prettyprinter.cc (right):
https://codereview.chromium.org/1315823002/diff/20001/src/prettyprinter.cc#newcode854
src/prettyprinter.cc:854: Print("");
I'd print "()" here.
https://codereview.chromium.org/1315823002/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [parser] partially revert "use-strict directives in function body affect init block" (issue 1300103005 by conr...@chromium.org)
lgtm https://codereview.chromium.org/1300103005/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Visit AST Property nodes as expressions in AstExpressionVisitor. (issue 1314843002 by bradnel...@google.com)
https://codereview.chromium.org/1314843002/diff/1/src/ast-expression-visitor.cc
File src/ast-expression-visitor.cc (right):
https://codereview.chromium.org/1314843002/diff/1/src/ast-expression-visitor.cc#newcode235
src/ast-expression-visitor.cc:235: void
AstExpressionVisitor::VisitYield(Yield* expr) {
Shouldn't this change as well?
https://codereview.chromium.org/1314843002/diff/1/src/ast-expression-visitor.cc#newcode241
src/ast-expression-visitor.cc:241: void
AstExpressionVisitor::VisitThrow(Throw* expr) {
And this one?
https://codereview.chromium.org/1314843002/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [es6] Correct length for functions with default parameters (issue 1311163002 by rossb...@chromium.org)
https://codereview.chromium.org/1311163002/diff/1/src/scopes.h
File src/scopes.h (right):
https://codereview.chromium.org/1311163002/diff/1/src/scopes.h#newcode133
src/scopes.h:133: bool is_optional, bool is_rest, bool* is_duplicate);
On 2015/08/24 17:51:00, adamk wrote:
Two bools in a row suggests to me that an enum would be better.
Given that is_optional and is_rest are mutually exclusive, they could
even be
part of the same enum, e.g.:
enum class ParameterType {
kNormal,
kOptional,
kRest
};
I realize that there's only a single caller, though, so another option
would be
to simply name the is_optional variable passed in the caller.
Named it at call site.
https://codereview.chromium.org/1311163002/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [es6] Remaining cases of parameter scopes for sloppy eval (issue 1303013007 by rossb...@chromium.org)
Friendly ping. https://codereview.chromium.org/1303013007/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Fix AstExpressionVisitor to correctly handle switch + for. (issue 1316633002 by bradnel...@google.com)
lgtm https://codereview.chromium.org/1316633002/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Fix AstExpressionVisitor to correctly handle switch + for. (issue 1316633002 by bradnel...@google.com)
https://codereview.chromium.org/1316633002/diff/1/src/ast-expression-visitor.cc File src/ast-expression-visitor.cc (right): https://codereview.chromium.org/1316633002/diff/1/src/ast-expression-visitor.cc#newcode116 src/ast-expression-visitor.cc:116: VisitExpression(label); Why is the label visited twice? https://codereview.chromium.org/1316633002/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [es6] Fix default parameters in arrow functions (issue 1314543005 by rossb...@chromium.org)
https://codereview.chromium.org/1314543005/diff/1/test/cctest/test-parsing.cc
File test/cctest/test-parsing.cc (right):
https://codereview.chromium.org/1314543005/diff/1/test/cctest/test-parsing.cc#newcode3750
test/cctest/test-parsing.cc:3750: // "({a} = {}) => {}",
On 2015/08/24 16:01:47, wingo wrote:
Perhaps add a pattern for ({a=42})=>{} too? Unless I am misreading
the grammar
of course. Same thing for array patterns with initializers. I don't
remember
what the state of the implementation is though.
Done.
https://codereview.chromium.org/1314543005/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [es6] Fix default parameters in arrow functions (issue 1314543005 by rossb...@chromium.org)
On 2015/08/24 16:01:48, wingo wrote:
lgtm with test nits
https://codereview.chromium.org/1314543005/diff/1/test/cctest/test-parsing.cc
File test/cctest/test-parsing.cc (right):
https://codereview.chromium.org/1314543005/diff/1/test/cctest/test-parsing.cc#newcode3750
test/cctest/test-parsing.cc:3750: // "({a} = {}) => {}",
Perhaps add a pattern for ({a=42})=>{} too? Unless I am misreading the
grammar
of course. Same thing for array patterns with initializers. I don't
remember
what the state of the implementation is though.
Will do.
AFAIU default parameters should work now so we probably also need
functional
tests, i.e. tests that also invoke the arrow function.
Those already exist, see harmony/default-parameters.js -- which is why I'm
still
surprised that the bogus code was never triggered before.
https://codereview.chromium.org/1314543005/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [es6] Implement default parameters (issue 1287063004 by rossb...@chromium.org)
https://codereview.chromium.org/1314543005 https://codereview.chromium.org/1287063004/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [es6] Implement default parameters (issue 1287063004 by rossb...@chromium.org)
https://codereview.chromium.org/1287063004/diff/60001/src/parser.cc
File src/parser.cc (right):
https://codereview.chromium.org/1287063004/diff/60001/src/parser.cc#newcode3916
src/parser.cc:3916: parser_->Check(Token::ASSIGN)) {
On 2015/08/24 14:10:10, wingo wrote:
How on earth does this work? This function is called after the arrow
function
arguments have been parsed, and its purpose is to transform the
already-parsed
expression into parameters. I can't see how this can possibly do the
right
thing.
You are right, it doesn't make sense. 8} Not sure what I was thinking.
Any idea why this is never hit by the tests?
https://codereview.chromium.org/1287063004/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [parser] partially revert "use-strict directives in function body affect init block" (issue 1300103005 by conr...@chromium.org)
https://codereview.chromium.org/1300103005/diff/11/src/expression-classifier.h
File src/expression-classifier.h (right):
https://codereview.chromium.org/1300103005/diff/11/src/expression-classifier.h#newcode235
src/expression-classifier.h:235: // parameter
Nit: period
https://codereview.chromium.org/1300103005/diff/11/src/preparser.cc
File src/preparser.cc (right):
https://codereview.chromium.org/1300103005/diff/11/src/preparser.cc#newcode270
src/preparser.cc:270: if (!scope_->HasSimpleParameters()) {
Nit: merge conditions
https://codereview.chromium.org/1300103005/diff/11/src/preparser.cc#newcode271
src/preparser.cc:271: // A block declaration scope as a child scope of a
function scope
Not sure how the comment relates to the code. Which block scope?
https://codereview.chromium.org/1300103005/diff/11/src/preparser.h
File src/preparser.h (right):
https://codereview.chromium.org/1300103005/diff/11/src/preparser.h#newcode2860
src/preparser.h:2860: bool is_simple =
arrow_formals_classifier.is_simple_parameter_list();
Nit: avoid the extra variable by storing to parameters.is_simple
directly.
https://codereview.chromium.org/1300103005/diff/11/test/cctest/test-parsing.cc
File test/cctest/test-parsing.cc (right):
https://codereview.chromium.org/1300103005/diff/11/test/cctest/test-parsing.cc#newcode6885
test/cctest/test-parsing.cc:6885: // A block declaration scope as a
child scope of a function scope
Same here.
https://codereview.chromium.org/1300103005/diff/11/test/mjsunit/harmony/destructuring.js
File test/mjsunit/harmony/destructuring.js (right):
https://codereview.chromium.org/1300103005/diff/11/test/mjsunit/harmony/destructuring.js#newcode752
test/mjsunit/harmony/destructuring.js:752: });
Better call this function!
https://codereview.chromium.org/1300103005/diff/11/test/mjsunit/harmony/destructuring.js#newcode759
test/mjsunit/harmony/destructuring.js:759: });
Same here
https://codereview.chromium.org/1300103005/diff/11/test/mjsunit/harmony/destructuring.js#newcode790
test/mjsunit/harmony/destructuring.js:790: });
And here
https://codereview.chromium.org/1300103005/diff/11/test/mjsunit/harmony/rest-params.js
File test/mjsunit/harmony/rest-params.js (right):
https://codereview.chromium.org/1300103005/diff/11/test/mjsunit/harmony/rest-params.js#newcode149
test/mjsunit/harmony/rest-params.js:149: "use strict";
This changes the test. Make sure that the caller remains in sloppy mode.
https://codereview.chromium.org/1300103005/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Debugger: use correct position for for-next expression statement. (issue 1310213002 by yang...@chromium.org)
lgtm https://codereview.chromium.org/1310213002/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [parser] partially revert "use-strict directives in function body affect init block" (issue 1300103005 by conr...@chromium.org)
You might want to consider changing the CL title, because the actual revert is not even in it, IIUC. https://codereview.chromium.org/1300103005/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Adding visitors to regurgitate expression types or reset them. (issue 1288773007 by bradnel...@google.com)
https://codereview.chromium.org/1288773007/diff/140001/src/typing-reset.cc File src/typing-reset.cc (right): https://codereview.chromium.org/1288773007/diff/140001/src/typing-reset.cc#newcode22 src/typing-reset.cc:22: TypingReseter* visitor = new (info->zone()) TypingReseter(info); On 2015/08/20 21:35:47, bradn wrote: By the way, a terminology question. You referred to this as heap-allocation, but I had the impression that usually refers to allocation in V8's GC heap? Whereas isn't this just in a Zone scoped to this particular compile? Or have I misunderstood something about Zones + GC heap? I was just being sloppy. Meant heap-allocated as opposed to stack-allocated. https://codereview.chromium.org/1288773007/diff/140001/test/cctest/expression-type-collector.h File test/cctest/expression-type-collector.h (right): https://codereview.chromium.org/1288773007/diff/140001/test/cctest/expression-type-collector.h#newcode13 test/cctest/expression-type-collector.h:13: // Macros to define checking of a tree walk. On 2015/08/20 21:35:47, bradn wrote: On 2015/08/20 16:34:27, rossberg wrote: > That's some fancy macro fu there! :) Not sure whether that's a compliment of an insult :-) Too much, or ok? Can't make up my mind. :) I think it's fine and useful for tests, but perhaps would be too much in "real" code. https://codereview.chromium.org/1288773007/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Parse arrow functions at proper precedence level (issue 1286383005 by wi...@igalia.com)
lgtm https://codereview.chromium.org/1286383005/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [es6] Parameter scopes for sloppy eval (issue 1292753007 by rossb...@chromium.org)
https://codereview.chromium.org/1292753007/diff/1/src/contexts.cc File src/contexts.cc (right): https://codereview.chromium.org/1292753007/diff/1/src/contexts.cc#newcode65 src/contexts.cc:65: return ext->IsSloppyBlockWithEvalContextExtension() On 2015/08/20 10:30:00, Michael Starzinger wrote: nit: As discussed offline, it is not immediately clear from the code that a block-context containing the new pair will always be a declaration scope (i.e. that pair->scope_info()->is_declaration_scope() will always hold). Can we leave a comment or DCHECK in that regard here? Done: added a comment here and a DCHECK to the factory function for SloppyBlockWithBla itself. https://codereview.chromium.org/1292753007/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [es6] Parameter scopes for sloppy eval (issue 1292753007 by rossb...@chromium.org)
On 2015/08/20 21:24:48, Dan Ehrenberg wrote:
Such a weird spec, but your implementation makes sense. I can't think of a
simpler way. Why don't the default parameter direct evals just run within
the
function's existing var scope? (Or, why 1JS?!)
Because the var scope is not in scope for parameters. Otherwise, e.g. this
function f() { return 1 }
function g(x = f()) {
// ...way down
var f = 2
}
would have a rather surprising result (g() === undefined). I'd actually be
fine
if this happened with var (just don't use it), but for legacy reasons, inner
functions are var bindings, too, unless inside a block.
https://codereview.chromium.org/1292753007/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Don't allocate AstTyper with the zone allocator. (issue 1303843003 by bradnel...@google.com)
lgtm https://codereview.chromium.org/1303843003/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Parse arrow functions at proper precedence level (issue 1286383005 by wi...@igalia.com)
https://codereview.chromium.org/1286383005/diff/20001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/1286383005/diff/20001/src/preparser.h#newcode2288 src/preparser.h:2288: result = Nit: unnecessary line break? https://codereview.chromium.org/1286383005/diff/20001/src/preparser.h#newcode2290 src/preparser.h:2290: // Patterns not allowed as rest parameters. There is no way we can Nit: insert "are". FWIW, Microsoft just proposed to allow patterns there in ES7. But no reason to worry about that now... https://codereview.chromium.org/1286383005/diff/20001/src/preparser.h#newcode2871 src/preparser.h:2871: // ValidateExpression(&arrow_formals_classifier, CHECK_OK); Hm? https://codereview.chromium.org/1286383005/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Tighten & check the BookmarkScope API contract a bit. (issue 1302893002 by vogelh...@chromium.org)
On 2015/08/20 16:26:31, vogelheim wrote: https://codereview.chromium.org/1302893002/diff/1/src/scanner.h File src/scanner.h (right): https://codereview.chromium.org/1302893002/diff/1/src/scanner.h#newcode367 src/scanner.h:367: if (my_bookmark_) scanner_->DropBookmark(); On 2015/08/20 15:45:12, rossberg wrote: > Hm, this _does_ change the semantics, doesn't it? It looks more correct, though. Yes, good observation. But I think the case where this makes a difference shouldn't happen in practice, so it shouldn't change behaviour overall. And if the case does happen, this should fix a bug. :) This is what I originally meant to implement. Would be good to know if it is supposed to happen. :) If not, this could perhaps be a DCHECK. https://codereview.chromium.org/1302893002/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Adding visitors to regurgitate expression types or reset them. (issue 1288773007 by bradnel...@google.com)
https://codereview.chromium.org/1288773007/diff/40001/src/ast-expression-visitor.h
File src/ast-expression-visitor.h (right):
https://codereview.chromium.org/1288773007/diff/40001/src/ast-expression-visitor.h#newcode26
src/ast-expression-visitor.h:26: void* operator new(size_t size, Zone*
zone) { return zone->New(size); }
On 2015/08/20 04:01:40, bradn wrote:
On 2015/08/19 12:06:37, titzer wrote:
> I don't think you want to define your own new here. I think you want
to extend
> ZoneObject and use "new (zone)". It's not safer, but it's the V8
way.
Not groking. But maybe I'm missing something basic :-)
I can't inherit from ZoneObject without doing multiple inheritance
here, as I
need to inherit from AstVisitor. Also, AstVisitor inherits from
Embedded, which
in debug mode has a clashing operator new.
Currently this inlines ZoneObject in this class, so for instance in
TypingReseter::Run() I can do new (zone).
Or are you suggesting I do placement new there, ie:
new (zone->New(sizeof(TypingReseter))) TypingReseter ?
FWIW, this is the pattern used in typing.h and a few other places.
Honestly, I don't know why typing.h does that. Why would one want to
store a visitor on the heap? You can just have it on the stack, as in
TypingReseter visitor(info);
visitor.VisitAll();
or even
TypingReseter(info).VisitAll();
https://codereview.chromium.org/1288773007/diff/140001/src/typing-reset.cc
File src/typing-reset.cc (right):
https://codereview.chromium.org/1288773007/diff/140001/src/typing-reset.cc#newcode22
src/typing-reset.cc:22: TypingReseter* visitor = new (info->zone())
TypingReseter(info);
No need to heap-allocate the visitor, AFAICT.
https://codereview.chromium.org/1288773007/diff/140001/test/cctest/expression-type-collector.cc
File test/cctest/expression-type-collector.cc (right):
https://codereview.chromium.org/1288773007/diff/140001/test/cctest/expression-type-collector.cc#newcode32
test/cctest/expression-type-collector.cc:32: ExpressionTypeCollector*
visitor =
Same here, no reason for heap allocation.
https://codereview.chromium.org/1288773007/diff/140001/test/cctest/expression-type-collector.h
File test/cctest/expression-type-collector.h (right):
https://codereview.chromium.org/1288773007/diff/140001/test/cctest/expression-type-collector.h#newcode13
test/cctest/expression-type-collector.h:13: // Macros to define checking
of a tree walk.
That's some fancy macro fu there! :)
https://codereview.chromium.org/1288773007/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Fix parsing of arrow function formal parameters (issue 1306583002 by wi...@igalia.com)
lgtm https://codereview.chromium.org/1306583002/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Tighten & check the BookmarkScope API contract a bit. (issue 1302893002 by vogelh...@chromium.org)
lgtm https://codereview.chromium.org/1302893002/diff/1/src/scanner.h File src/scanner.h (right): https://codereview.chromium.org/1302893002/diff/1/src/scanner.h#newcode367 src/scanner.h:367: if (my_bookmark_) scanner_->DropBookmark(); Hm, this _does_ change the semantics, doesn't it? It looks more correct, though. https://codereview.chromium.org/1302893002/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Unify symbols sharing across native scripts and runtime. (issue 1293493004 by yang...@chromium.org)
LGTM It's sad that this makes the use of private symbols unmodular. But I guess uniformity wins... https://codereview.chromium.org/1293493004/diff/1/src/array-iterator.js File src/array-iterator.js (right): https://codereview.chromium.org/1293493004/diff/1/src/array-iterator.js#newcode16 src/array-iterator.js:16: var ArrayIterationKindSymbol = I don't feel too strongly, but don't we typically use capitalised names for functions only? https://codereview.chromium.org/1293493004/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Rename ParserInfo::function() and CompilationInfo::function() to literal(). (issue 1301583005 by tit...@chromium.org)
LGTM (rubberstamp) https://codereview.chromium.org/1301583005/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Align PreParser for loop early error-checking with Parser (issue 1290193003 by ad...@chromium.org)
lgtm https://codereview.chromium.org/1290193003/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Make some foo.h headers usable without foo-inl.h header. (issue 1290743005 by mstarzin...@chromium.org)
lgtm https://codereview.chromium.org/1290743005/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [es6] Make assignment to new.target an early ReferenceError (issue 1290013002 by ad...@chromium.org)
lgtm
https://codereview.chromium.org/1290013002/diff/20001/test/mjsunit/harmony/new-target.js
File test/mjsunit/harmony/new-target.js (right):
https://codereview.chromium.org/1290013002/diff/20001/test/mjsunit/harmony/new-target.js#newcode390
test/mjsunit/harmony/new-target.js:390: assertThrows(function() {
Function("new.target = 42"); }, ReferenceError);
Could add a case for at least one compound assignment op.
https://codereview.chromium.org/1290013002/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Remove old webkit Object-getOwnPropertyNames test (issue 1275423006 by ad...@chromium.org)
On 2015/08/11 21:20:19, adamk wrote: What do you think? LGTM. Never quite got the point of this test. https://codereview.chromium.org/1275423006/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [es6] Implement default parameters (issue 1287063004 by rossb...@chromium.org)
https://codereview.chromium.org/1287063004/diff/1/src/parser.cc
File src/parser.cc (right):
https://codereview.chromium.org/1287063004/diff/1/src/parser.cc#newcode4328
src/parser.cc:4328: Expression* param =
On 2015/08/12 18:22:59, adamk wrote:
I think this should be called "initializer"
That would confuse it with the actual initializer expression. I renamed
it to initial_value instead, is that okay?
https://codereview.chromium.org/1287063004/diff/1/src/parser.cc#newcode4342
src/parser.cc:4342: parameter.pattern, parameter.pattern->position(),
param);
On 2015/08/12 18:22:59, adamk wrote:
The above-suggested renaming would make the callsite clearer.
Done.
https://codereview.chromium.org/1287063004/diff/1/src/preparser.h
File src/preparser.h (left):
https://codereview.chromium.org/1287063004/diff/1/src/preparser.h#oldcode3625
src/preparser.h:3625: bool is_rest, FormalParametersT* parameters,
On 2015/08/12 18:22:59, adamk wrote:
This rest refactoring seems separate, could it be broken into another
patch if
it's a prerequisite or dropped if it's a separate concern?
Split into separate CL: https://codereview.chromium.org/1286133003
https://codereview.chromium.org/1287063004/diff/1/test/mjsunit/harmony/default-parameters.js
File test/mjsunit/harmony/default-parameters.js (right):
https://codereview.chromium.org/1287063004/diff/1/test/mjsunit/harmony/default-parameters.js#newcode84
test/mjsunit/harmony/default-parameters.js:84: (function
TestParameterScoping() {
On 2015/08/12 18:22:59, adamk wrote:
This test looks like it could use some cases with more complex evals,
e.g.,
those that declare variables with "var" and "let" (in both strict and
sloppy
modes).
Since these don't actually work correctly yet I just added a TODO.
https://codereview.chromium.org/1287063004/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [strong] Fix weird work-around for magic truncating int conversions. (issue 1288313003 by bmeu...@chromium.org)
lgtm https://codereview.chromium.org/1288313003/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [es6] Implement default parameters (issue 1287063004 by rossb...@chromium.org)
https://codereview.chromium.org/1287063004/diff/1/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/1287063004/diff/1/src/parser.cc#newcode3908 src/parser.cc:3908: // TODO(rossberg): allow initializers Need to come up with a test that actually triggers this. https://codereview.chromium.org/1287063004/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Do not save script object on the class constructor. (issue 1290703002 by yang...@chromium.org)
lgtm https://codereview.chromium.org/1290703002/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Group lexical context variables for faster look up. (issue 1281883002 by yang...@chromium.org)
LGTM, I'm fine with landing this as a temporary hack, but can we have a couple of comments (and perhaps TODOs) clarifying why it was introduced? CC'ing Benedikt. https://codereview.chromium.org/1281883002/diff/1/src/scopeinfo.cc File src/scopeinfo.cc (right): https://codereview.chromium.org/1281883002/diff/1/src/scopeinfo.cc#newcode591 src/scopeinfo.cc:591: int total_count = scope_info->ContextLocalCount(); So, this means that lexical variable lookup does not use the cache anymore? That is going to make it worse in some cases. At least add a TODO. https://codereview.chromium.org/1281883002/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [es6] Use strict arguments objects for destructured parameters (issue 1278783002 by rossb...@chromium.org)
On 2015/08/06 17:12:43, adamk wrote: Can you add some tests with arrow functions? Want to make sure that's covered, as some of the logic upstream is different (though should all flow through the same place). That's funny: I actually had tests for arrow functions, and then spent an hour today trying to figure out why they don't "work". Until I realised that they (and me) were stupid, because arrows don't bind `arguments`. :) https://codereview.chromium.org/1278783002/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [es6] re-implement rest parameters via desugaring (issue 1272673003 by caitpotte...@gmail.com)
I also tried digging into it, but without much success. Something must be pointing into the wild, but I couldn't find out what. Some cases fail with an invalid access. I'd try to breakpoint on those in the debugger and try to figure out what there pointing at and where it is coming from. Maybe that gives a hint. The next thing you could try is not invoking DeclareParameter on the rest param at all, and just desugar using a temporary instead. In fact, it should be possible to make it such that all traces of a rest parameter effectively disappear after desugaring. (The only thing you probably need to remember is whether the parameter list was simple, for allocating the right arguments object later.) You could also try to temporarily replace the desugaring with a nop. If that doesn't make the problem go away then maybe something is f-ed up with variable/temporary allocation? Maybe there also is some weird problem with doing the desugaring after processing the body. Could be worth a shot trying to undo that (with the TC39 resolution it doesn't seem necessary anymore anyway). https://codereview.chromium.org/1272673003/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
Re: [v8-dev] Re: Ensure `String.prototype.normalize.length` is `0` (issue 1274653002 by math...@qiwi.be)
On 5 August 2015 at 22:18, Adam Klein wrote: > On Wed, Aug 5, 2015 at 7:23 AM, wrote: > >> >> https://codereview.chromium.org/1274653002/diff/1/src/i18n.js >> File src/i18n.js (right): >> >> https://codereview.chromium.org/1274653002/diff/1/src/i18n.js#newcode2019 >> src/i18n.js:2019: %FunctionSetLength(StringNormalizeJS, 0); >> I'd prefer avoiding the runtime call and instead define the function >> with 0 arguments, using >> >> var form = %_Argument(0) >> >> in the body. Same in string.js. > > > Because I've sometimes given the opposite advice, I'm curious for your > thinking here. Why should we prefer one over the other? The runtime call > happens at snapshot time, so there's no runtime cost. > My rule of thumb is: minimize magic. Arguably, %FunctionSetLength is more magic than %_Arguments, which really just is a fast version of `arguments`. Also, with respect to arity of built-ins, I envision that we'll eventually be able to use ES6 syntax to get rid of any %-magic. I'd consider the %_Arg version a step closer in that direction, since it at least localises the magic inside the function body. All not particularly strong points, but you asked. :) /Andreas -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [strong] dot prototypes of class literals should be strong objects (issue 1270423003 by conr...@chromium.org)
lgtm https://codereview.chromium.org/1270423003/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Ensure `String.prototype.normalize.length` is `0` (issue 1274653002 by math...@qiwi.be)
lgtm https://codereview.chromium.org/1274653002/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [strong] Refactor out separate strong runtime call for class objects (issue 1270393002 by conr...@chromium.org)
lgtm https://codereview.chromium.org/1270393002/diff/1/src/runtime/runtime-classes.cc File src/runtime/runtime-classes.cc (right): https://codereview.chromium.org/1270393002/diff/1/src/runtime/runtime-classes.cc#newcode266 src/runtime/runtime-classes.cc:266: RETURN_FAILURE_ON_EXCEPTION(isolate, JSObject::Freeze(prototype)); How about adding a DCHECK(prototype->map()->is_strong()) here? https://codereview.chromium.org/1270393002/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Ensure `String.prototype.normalize.length` is `0` (issue 1274653002 by math...@qiwi.be)
https://codereview.chromium.org/1274653002/diff/1/src/i18n.js File src/i18n.js (right): https://codereview.chromium.org/1274653002/diff/1/src/i18n.js#newcode2019 src/i18n.js:2019: %FunctionSetLength(StringNormalizeJS, 0); I'd prefer avoiding the runtime call and instead define the function with 0 arguments, using var form = %_Argument(0) in the body. Same in string.js. https://codereview.chromium.org/1274653002/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [es6] Remove Scanner and Parser flags for harmony_modules (issue 1262913003 by ad...@chromium.org)
LGTM, modulo build problem https://codereview.chromium.org/1262913003/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Delete --harmony-unicode flag (issue 1271073002 by ad...@chromium.org)
lgtm
https://codereview.chromium.org/1271073002/diff/1/src/scanner.cc
File src/scanner.cc (right):
https://codereview.chromium.org/1271073002/diff/1/src/scanner.cc#newcode1078
src/scanner.cc:1078: // Accept both \u and \u{xx}.
Nit: reformat.
https://codereview.chromium.org/1271073002/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Delete --harmony-computed-property-names flag (issue 1273543002 by ad...@chromium.org)
lgtm https://codereview.chromium.org/1273543002/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [es6] Implement proper TDZ for parameters (issue 1259283002 by rossb...@chromium.org)
https://codereview.chromium.org/1259283002/diff/40001/src/parser.h
File src/parser.h (right):
https://codereview.chromium.org/1259283002/diff/40001/src/parser.h#newcode556
src/parser.h:556: const Parameter& at(int i) const { return params[i]; }
On 2015/08/04 17:25:04, adamk wrote:
On 2015/08/04 16:46:25, caitp wrote:
> minor question: Is there a sort of system in place for deciding
whether to
name
> methods with PascalCase vs camelCase vs snake_case? All three seem
to be used
in
> V8, sometimes within the same classes (as here). The style-guide
doesn't seem
> super clear on this.
camelCase shouldn't be used in C++ (and I can't think of a place where
it is),
though we do use it in JS. For the other two, see this bit of the
Google C++
style guide:
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Names
Short answer: trivial inlined members (such as getters/setters, or
this at()
case) may have snake_case names (the idea being to signal that they
are
trivial). Any non-trivial method uses PascalCase.
What Adam said. And yes, V8 is rather inconsistent with this convention.
OTOH, I'm not even convinced the convention makes a lot of sense.
https://codereview.chromium.org/1259283002/diff/40001/src/parser.h#newcode1319
src/parser.h:1319: parser_->allow_harmony_rest_parameters() ||
is_simple);
On 2015/08/04 16:46:25, caitp wrote:
since these are just DCHECKs, why not make these more robust? Ensure
the rest
flag is used for rest, destructuring flag used for patterns. Might
avoid some
false DCHECK successes?
Good idea. Will incorporate that into the next CL.
https://codereview.chromium.org/1259283002/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [es6] Implement proper TDZ for parameters (issue 1259283002 by rossb...@chromium.org)
Addressed the arity issue by separating out a FormalParametersBase class that does not have an arity field, so that it only exists in the preparser. It is now updated in PreParserTraits::AddFormalParameter. https://codereview.chromium.org/1259283002/diff/1/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/1259283002/diff/1/src/parser.cc#newcode1205 src/parser.cc:1205: ParseFormalParameter(is_rest, &formals, &formals_classifier, &ok); On 2015/08/03 18:43:51, adamk wrote: Check ok before declaring? Done. https://codereview.chromium.org/1259283002/diff/1/src/parser.cc#newcode src/parser.cc:: !scope_->is_declaration_scope() ? LET : VAR; On 2015/08/03 18:43:51, adamk wrote: This looks unrelated to the rest of the change; maybe it's further fixup from the broadening of declaration scope definition? You are right, I overlooked this one when splitting up the CL. It is needed because there was some test started failing with this CL otherwise. I can separate it out if you want. https://codereview.chromium.org/1259283002/diff/1/src/parser.h File src/parser.h (right): https://codereview.chromium.org/1259283002/diff/1/src/parser.h#newcode1331 src/parser.h:1331: DCHECK(parameters->arity == parameters->params.length()); On 2015/08/03 18:43:52, adamk wrote: Nit: DCHECK_EQ Done. https://codereview.chromium.org/1259283002/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Fix build error (missing cast to void*) (issue 1263043004 by rossb...@chromium.org)
On 2015/08/04 15:17:58, mtbrandyberry wrote:
On 2015/08/04 14:24:43, commit-bot: I haz the power wrote:
> Patchset 1 (id:??) landed as
> https://crrev.com/479e0c034736f7aab301daab190f5d8a58d20d7d
> Cr-Commit-Position: refs/heads/master@{#30001}
Another instance of this same issue exists on line 916 in
Scope::Print(int n)
as
well.
Oops, thanks! Fixed here:
https://codereview.chromium.org/1264233005/
https://codereview.chromium.org/1263043004/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Rename IsSimdObject assembly intrinsic. (issue 1253103006 by bbu...@chromium.org)
LGTM, thanks! https://codereview.chromium.org/1253103006/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [es6] Refactor FormalParameter (issue 1259013003 by rossb...@chromium.org)
https://codereview.chromium.org/1259013003/diff/1/src/parser.h File src/parser.h (right): https://codereview.chromium.org/1259013003/diff/1/src/parser.h#newcode559 src/parser.h:559: DCHECK(arity == params.length()); On 2015/08/03 18:20:23, adamk wrote: DCHECK_EQ. Done. Also, it will probably be more clear after the followup, but it seems sad that the only way to check this consistency is a DCHECK. Especially since the incrementing code is now duplicated between preparser and parser. My thinking was that the parser/preparser duplication should go away eventually, once statement parsing is unified. But the FormalParameter will stay separate. So you want to move as little of the logic there as possible. But maybe you are right, I'll reconsider in the follow-up CL. https://codereview.chromium.org/1259013003/diff/1/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/1259013003/diff/1/src/preparser.h#newcode3673 src/preparser.h:3673: DCHECK(parameters->arity == 0); On 2015/08/03 18:20:23, adamk wrote: DCHECK_EQ Done. https://codereview.chromium.org/1259013003/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: V8: Add SIMD functions. (issue 1230343003 by bbu...@chromium.org)
LGTM, but Dan should have a look as well. https://codereview.chromium.org/1230343003/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: SIMD.js Add the other SIMD Phase 1 types. (issue 1250733005 by bbu...@chromium.org)
https://codereview.chromium.org/1250733005/diff/310001/src/macros.py File src/macros.py (right): https://codereview.chromium.org/1250733005/diff/310001/src/macros.py#newcode101 src/macros.py:101: macro IS_SIMD_OBJECT(arg) = (%_IsSimdObject(arg)); Please revert this renaming. SIMD values are not objects, so it is misleading here (even though we conflate terminology on lower levels). https://codereview.chromium.org/1250733005/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: SIMD.js Add the other SIMD Phase 1 types. (issue 1250733005 by bbu...@chromium.org)
On 2015/08/03 14:31:31, bbudge wrote:
On 2015/08/03 14:10:34, titzer wrote:
> On 2015/08/03 13:54:42, Yang wrote:
> > On 2015/08/03 13:03:06, commit-bot: I haz the power wrote:
> > > Patchset 11 (id:??) landed as
> > > https://crrev.com/7b9670b63b486ba3b6f8a569552d307282dbccfd
> > > Cr-Commit-Position: refs/heads/master@{#29974}
> >
> > Well by simply marking NOPRESUBMIT=true, other test bots in the CQ
complain
> > about the same issue
> >
>
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/4638/steps/presubmit/logs/stdio
>
> Yes, please make sure the CQ runs all of the trybots, including
v8-presubmit.
>
> This CL changed a lot of compiler code in both Crankshaft and fullcode;
it's
> normal to loop in a compiler team person or two for the review in such
cases.
Thanks for the fix Ben. Sorry for any trouble this may have caused. I
would
definitely appreciate a compiler person reviewing the change. If you
prefer,
we
can revert this and review it properly.
No point in reverting, I think -- you can address comments in a follow-up
CLs.
But in general, when you change hundreds of lines of code after an LGTM,
it's
better to wait for another round of review before landing. ;)
Added Ben as an additional reviewer for the compiler-related changes in
this CL.
https://codereview.chromium.org/1250733005/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Add a --harmony-object-observe runtime flag (on by default) (issue 1257063003 by ad...@chromium.org)
lgtm https://codereview.chromium.org/1257063003/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Ship --harmony-new-target (issue 1267773009 by ad...@chromium.org)
LGTM, I'm okay with this. https://codereview.chromium.org/1267773009/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [es6] new.target should not be shadowable in a with scope (issue 1259183005 by ad...@chromium.org)
lgtm https://codereview.chromium.org/1259183005/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Create function name const assignment after parsing language mode. (issue 1260053004 by yang...@chromium.org)
https://codereview.chromium.org/1260053004/diff/20001/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/1260053004/diff/20001/src/parser.cc#newcode4436 src/parser.cc:4436: VariableMode fvar_mode = use_strict_const ? CONST : CONST_LEGACY; Wait, I thought the idea was that this should always be CONST. In which case you don't need to bother deferring the creation of the declaration. https://codereview.chromium.org/1260053004/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: SIMD.js Add the other SIMD Phase 1 types. (issue 1250733005 by bbu...@chromium.org)
On 2015/07/30 16:21:27, bbudge wrote: On 2015/07/30 15:50:39, rossberg wrote: > On 2015/07/30 15:34:58, adamk wrote: > > https://codereview.chromium.org/1250733005/diff/230001/src/macros.py > > File src/macros.py (right): > > > > > https://codereview.chromium.org/1250733005/diff/230001/src/macros.py#newcode101 > > src/macros.py:101: macro IS_SIMD_VALUE(arg)= (%_IsSimdValue(arg)); > > On 2015/07/30 15:12:24, rossberg wrote: > > > I'm surprised this actually works! See above, you should have written > > > %IsSimdValue here. > > > > Drive-by explanation of why this works: since a recent refactor (by Sven I > think > > it was), the "%_" form works for all runtime calls, and the backends fall back > > to the runtime if they don't have an implementation of a given intrinsic. > > Hm, was there a particular motivation for that change? Seems to make intent less > clear and accidents go unnoticed. I'd be happy to implement the assembly intrinsic. Is there an example or some documentation for the extra work that needs to be done? Documentation? For V8? Are you sure you have already lowered your expectations about V8 far enough? :) But it should work to follow the model of other such intrinsics, e.g. ClassOf. The main annoyance is that you'll have to implement it for all the gazillion compilers times architectures. https://codereview.chromium.org/1250733005/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: SIMD.js Add the other SIMD Phase 1 types. (issue 1250733005 by bbu...@chromium.org)
On 2015/07/30 15:34:58, adamk wrote: https://codereview.chromium.org/1250733005/diff/230001/src/macros.py File src/macros.py (right): https://codereview.chromium.org/1250733005/diff/230001/src/macros.py#newcode101 src/macros.py:101: macro IS_SIMD_VALUE(arg)= (%_IsSimdValue(arg)); On 2015/07/30 15:12:24, rossberg wrote: > I'm surprised this actually works! See above, you should have written > %IsSimdValue here. Drive-by explanation of why this works: since a recent refactor (by Sven I think it was), the "%_" form works for all runtime calls, and the backends fall back to the runtime if they don't have an implementation of a given intrinsic. Hm, was there a particular motivation for that change? Seems to make intent less clear and accidents go unnoticed. https://codereview.chromium.org/1250733005/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: SIMD.js Add the other SIMD Phase 1 types. (issue 1250733005 by bbu...@chromium.org)
LGTM modulo comments. Let's see how this version performs. We can always roll back. https://codereview.chromium.org/1250733005/diff/210001/src/macros.py File src/macros.py (right): https://codereview.chromium.org/1250733005/diff/210001/src/macros.py#newcode136 src/macros.py:136: macro IS_SIMD_VALUE(arg) = (IS_FLOAT32X4(arg) || IS_INT32X4(arg) || IS_BOOL32X4(arg) || IS_INT16X8(arg) || IS_BOOL16X8(arg) || IS_INT8X16(arg) || IS_BOOL8X16(arg)); On 2015/07/30 13:46:58, bbudge wrote: On 2015/07/29 14:37:55, rossberg wrote: > Hm, this seems rather expensive. It is used on some potentially > performance-relevant paths, so there should probably be a %_IsSimdValue > intrinsic to make it efficient. I'm fine with doing that in a follow-up CL, but > better be prepared that landing the CL as is might affect some benchmarks. I don't like those perf bugs, so I'll use your suggestion now. Done. Ah, note the difference between assembly intrinsics (%_XY) and C intrinsics (%XY). The former are recognised as distinguished AST nodes for which code generators generate inline assembly directly. The latter create runtime calls, which are expensive. I actually had the former in mind, but that involves some work. I doubt the latter will perform much better than the multi-disjunction. But I guess we'll find out. :) https://codereview.chromium.org/1250733005/diff/210001/src/runtime.js File src/runtime.js (right): https://codereview.chromium.org/1250733005/diff/210001/src/runtime.js#newcode106 src/runtime.js:106: if (IS_SYMBOL(y) || IS_SIMD_VALUE(y)) return 1; // not equal On 2015/07/30 13:46:58, bbudge wrote: I moved these primitive checks which always return "not equal" to the end of their blocks, since they shouldn't be encountered in real or benchmark code. WDYT? Fine with me. https://codereview.chromium.org/1250733005/diff/230001/src/macros.py File src/macros.py (right): https://codereview.chromium.org/1250733005/diff/230001/src/macros.py#newcode101 src/macros.py:101: macro IS_SIMD_VALUE(arg)= (%_IsSimdValue(arg)); I'm surprised this actually works! See above, you should have written %IsSimdValue here. https://codereview.chromium.org/1250733005/diff/230001/src/runtime/runtime-simd.cc File src/runtime/runtime-simd.cc (right): https://codereview.chromium.org/1250733005/diff/230001/src/runtime/runtime-simd.cc#newcode167 src/runtime/runtime-simd.cc:167: return Smi::FromInt(result ? EQUAL : NOT_EQUAL); It is a bit strange to return an int here and Booleans for the other functions. https://codereview.chromium.org/1250733005/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: VectorICs: --print-ast now prints allocated vector slots (issue 1264823003 by mvstan...@chromium.org)
lgtm https://codereview.chromium.org/1264823003/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Move final parts of class literal setup into a single runtime call (issue 1266573003 by conr...@chromium.org)
lgtm https://codereview.chromium.org/1266573003/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: SIMD.js Add the other SIMD Phase 1 types. (issue 1250733005 by bbu...@chromium.org)
Mostly looking good, but I wonder about the cost of the IS_SIMD_VALUE
predicate.
https://codereview.chromium.org/1250733005/diff/210001/src/harmony-simd.js
File src/harmony-simd.js (right):
https://codereview.chromium.org/1250733005/diff/210001/src/harmony-simd.js#newcode35
src/harmony-simd.js:35: return %CreateFloat32x4(TO_NUMBER_INLINE(c0),
TO_NUMBER_INLINE(c1), TO_NUMBER_INLINE(c2), TO_NUMBER_INLINE(c3));
Nit: line length
https://codereview.chromium.org/1250733005/diff/210001/src/harmony-simd.js#newcode90
src/harmony-simd.js:90:
//---
Would it be possible to avoid all the repetition by using a macro? (See
e.g. typedarray.js)
https://codereview.chromium.org/1250733005/diff/210001/src/heap/heap.cc
File src/heap/heap.cc (right):
https://codereview.chromium.org/1250733005/diff/210001/src/heap/heap.cc#newcode3119
src/heap/heap.cc:3119: AllocationResult Heap::AllocateFloat32x4(float
lanes[4],
Macrofication might help here, too.
https://codereview.chromium.org/1250733005/diff/210001/src/macros.py
File src/macros.py (right):
https://codereview.chromium.org/1250733005/diff/210001/src/macros.py#newcode136
src/macros.py:136: macro IS_SIMD_VALUE(arg) = (IS_FLOAT32X4(arg)
|| IS_INT32X4(arg) || IS_BOOL32X4(arg) || IS_INT16X8(arg) ||
IS_BOOL16X8(arg) || IS_INT8X16(arg) || IS_BOOL8X16(arg));
Hm, this seems rather expensive. It is used on some potentially
performance-relevant paths, so there should probably be a %_IsSimdValue
intrinsic to make it efficient. I'm fine with doing that in a follow-up
CL, but better be prepared that landing the CL as is might affect some
benchmarks.
https://codereview.chromium.org/1250733005/diff/210001/src/macros.py#newcode137
src/macros.py:137: macro IS_SIMD_VALUE_WRAPPER(arg) =
(IS_FLOAT32X4_WRAPPER(arg) || IS_INT32X4_WRAPPER(arg) ||
IS_BOOL32X4_WRAPPER(arg) || IS_INT16X8_WRAPPER(arg) ||
IS_BOOL16X8_WRAPPER(arg) || IS_INT8X16_WRAPPER(arg) ||
IS_BOOL8X16_WRAPPER(arg));
Is this actually used?
https://codereview.chromium.org/1250733005/diff/210001/src/objects.cc
File src/objects.cc (right):
https://codereview.chromium.org/1250733005/diff/210001/src/objects.cc#newcode102
src/objects.cc:102: return MaybeHandle();
Nit: the return here is redundant.
https://codereview.chromium.org/1250733005/diff/210001/src/runtime.js
File src/runtime.js (right):
https://codereview.chromium.org/1250733005/diff/210001/src/runtime.js#newcode136
src/runtime.js:136: } else if (IS_FLOAT32X4(x)) {
Guard these by a (cheaper) IS_SIMD_VALUE test.
https://codereview.chromium.org/1250733005/diff/210001/src/runtime.js#newcode189
src/runtime.js:189: if (IS_FLOAT32X4(this) && IS_FLOAT32X4(x))
Same here.
https://codereview.chromium.org/1250733005/diff/210001/src/runtime.js#newcode932
src/runtime.js:932: if (IS_FLOAT32X4(x)) return %Float32x4SameValue(x,
y);
And here.
https://codereview.chromium.org/1250733005/diff/210001/src/runtime.js#newcode949
src/runtime.js:949: if (IS_FLOAT32X4(x)) return
%Float32x4SameValueZero(x, y);
And here.
https://codereview.chromium.org/1250733005/diff/210001/test/cctest/test-simd.cc
File test/cctest/test-simd.cc (right):
https://codereview.chromium.org/1250733005/diff/210001/test/cctest/test-simd.cc#newcode14
test/cctest/test-simd.cc:14: TEST(SameValue) {
Any reason not to test the other types, too?
https://codereview.chromium.org/1250733005/diff/210001/test/mjsunit/harmony/simd.js
File test/mjsunit/harmony/simd.js (right):
https://codereview.chromium.org/1250733005/diff/210001/test/mjsunit/harmony/simd.js#newcode277
test/mjsunit/harmony/simd.js:277: // SIMD values should not be equal to
any other kind of object.
Can you add tests involving two different SIMD types?
https://codereview.chromium.org/1250733005/diff/210001/test/mjsunit/harmony/simd.js#newcode324
test/mjsunit/harmony/simd.js:324: function TestSameValue(type, lanes) {
Same here.
https://codereview.chromium.org/1250733005/diff/210001/test/mjsunit/harmony/simd.js#newcode371
test/mjsunit/harmony/simd.js:371: var a = createInstance(type), b =
createInstance(type);
Again it would be good to check the matrix of SIMD types, and also
include a couple of non-SIMD values on either side.
https://codereview.chromium.org/1250733005/diff/210001/test/mjsunit/harmony/simd.js#newcode423
test/mjsunit/harmony/simd.js:423: test(set, instance);
This now only tests for a zeroed value, which seems a bit limited.
https://codereview.chromium.org/1250733005/diff/210001/test/mjsunit/harmony/simd.js#newcode451
test/mjsunit/harmony/simd.js:451: test(map, instance, {});
Similarly here.
https://codereview.chromium.org/1250733005/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups
[v8-dev] Re: Optimize ToString and NonStringToString. (issue 1256323004 by bbu...@chromium.org)
lgtm https://codereview.chromium.org/1256323004/diff/20001/src/runtime.js File src/runtime.js (left): https://codereview.chromium.org/1256323004/diff/20001/src/runtime.js#oldcode769 src/runtime.js:769: if (IS_SYMBOL_WRAPPER(x)) throw MakeTypeError(kSymbolToPrimitive); On 2015/07/28 19:07:30, bbudge wrote: Is this error part of the Spec? If so, we have to keep this line. No, the spec doesn't even recognise the existence of error messages. https://codereview.chromium.org/1256323004/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Optimise ToNumber and NonNumberToNumber. (issue 1260273002 by bbu...@chromium.org)
lgtm https://codereview.chromium.org/1260273002/diff/20001/src/runtime.js File src/runtime.js (left): https://codereview.chromium.org/1260273002/diff/20001/src/runtime.js#oldcode796 src/runtime.js:796: if (IS_FLOAT32X4(x)) throw MakeTypeError(kSimdToNumber); Perhaps add a comment explaining the absence of these cases. https://codereview.chromium.org/1260273002/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Version 4.5.103.14 (cherry-pick) (issue 1258883002 by mvstan...@chromium.org)
lgtm https://codereview.chromium.org/1258883002/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [es6] Some renamings and minor clean-ups in parameter parsing (issue 1247443004 by rossb...@chromium.org)
https://codereview.chromium.org/1247443004/diff/1/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/1247443004/diff/1/src/parser.cc#newcode4306 src/parser.cc:4306: if (parameters.has_rest) return false; On 2015/07/23 11:12:48, Michael Starzinger wrote: This semantic change is intended I assume? Actually, no. This slipped in when I split up the CL. Thanks for catching! https://codereview.chromium.org/1247443004/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Fix check for a date with a 24th hour (issue 1240093005 by hichris...@gmail.com)
lgtm https://codereview.chromium.org/1240093005/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [es6] Fix function context check for super and new.target (issue 1244423003 by rossb...@chromium.org)
Also added a test for super calls. https://codereview.chromium.org/1244423003/diff/1/src/scopes.h File src/scopes.h (right): https://codereview.chromium.org/1244423003/diff/1/src/scopes.h#newcode486 src/scopes.h:486: // Find the first function, script, or eval scope. This is the scope On 2015/07/22 20:55:50, adamk wrote: Can you fully fix this comment while you're here to include the declaration block scopes? Done. https://codereview.chromium.org/1244423003/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Version 4.5.103.11 (cherry-pick) (issue 1251933002 by jkumme...@chromium.org)
lgtm https://codereview.chromium.org/1251933002/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [es6] re-implement rest parameters via desugaring (issue 1235153006 by caitpotte...@gmail.com)
Btw, can't you remove rest_parameter_ and rest_index_ from Scope now? https://codereview.chromium.org/1235153006/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: [es6] re-implement rest parameters via desugaring (issue 1235153006 by caitpotte...@gmail.com)
lgtm https://codereview.chromium.org/1235153006/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Fix check for a date with a 24th hour (issue 1240093005 by hichris...@gmail.com)
Looks good, just a nit.
https://codereview.chromium.org/1240093005/diff/70001/src/dateparser.cc
File src/dateparser.cc (right):
https://codereview.chromium.org/1240093005/diff/70001/src/dateparser.cc#newcode84
src/dateparser.cc:84: if (hour == 24 && minute == 0 && second == 0 &&
millisecond == 0) {
Invert to (hour != 24 || minute != 0 || second != 0 || millisecond != 0)
and drop the else branch.
https://codereview.chromium.org/1240093005/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[v8-dev] Re: Remove unnecessary coupling between Promise tests and Object.observe (issue 1246933002 by ad...@chromium.org)
https://codereview.chromium.org/1246933002/diff/1/test/mjsunit/es6/promises.js File test/mjsunit/es6/promises.js (right): https://codereview.chromium.org/1246933002/diff/1/test/mjsunit/es6/promises.js#newcode90 test/mjsunit/es6/promises.js:90: Promise.resolve().then( On 2015/07/21 16:18:08, adamk wrote: On 2015/07/21 10:25:37, rossberg wrote: > Some of the tests now become a bit circular (relying on the correctness of > Promise.resolve/then to test the correctness of Promise.resolve/then). But I > don't have a better idea either. I could switch them all to use %EnqueueMicrotask, if you'd prefer. Aw, of course. For some reason I though that can't be done, but I don't know what I was thinking. :\ Please do. https://codereview.chromium.org/1246933002/ -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
