Allow multiple function literals to be assigned to the same var / property.

Allow multiple function literals to be assigned to the same var / property.

In such a case all functions get the same name. I think it's a good performance / usability tradeoff. In case a developer wants more clarity, it's up to him to give names to functions.

Committed: http://code.google.com/p/v8/source/detail?r=1727

[Kevin Millikin (Chromium), 2009-04-16 08:06:58]

LGTM, with some suggestions.

http://codereview.chromium.org/67168/diff/1/2
File src/func-name-inferrer.cc (right):

http://codereview.chromium.org/67168/diff/1/2#newcode68
Line 68: return;
MaybeInferFunctionName is only called from InferAndLeave, which is inlined.  It
might make sense to move the check and early bailout into InferAndLeave.

http://codereview.chromium.org/67168/diff/1/2#newcode74
Line 74: funcs_to_infer_.Clear();
Clearing the list deletes the backing buffer, which then gets reallocated on the
first subsequent insertion and resized on the second.  You might consider
changing the loop to 'while (!funcs_to_infer_.is_empty()) ...' and use
List::RemoveLast and FuncNameInferrer::set_inferred_name in the body.

http://codereview.chromium.org/67168/diff/1/3
File src/func-name-inferrer.h (right):

http://codereview.chromium.org/67168/diff/1/3#newcode60
Line 60: void Leave() {
I don't think Leave is every called explicitly (only by InferAndLeave?), so you
might just inline it there and remove it from the API.

http://codereview.chromium.org/67168/diff/1/4
File src/rewriter.cc (right):

http://codereview.chromium.org/67168/diff/1/4#newcode285
Line 285: scoped_fni.Enter();
There is also a call to ScopedFuncNameInferrer::Enter in
AstOptimizer::VisitCallRuntime that is currently guarded by checking that the
expression is a function literal.  Do you want to allow other expression types
there as well?

[Lasse Reichstein, 2009-04-16 08:31:23]

Drive-by comment.

http://codereview.chromium.org/67168/diff/1/2
File src/func-name-inferrer.cc (right):

http://codereview.chromium.org/67168/diff/1/2#newcode74
Line 74: funcs_to_infer_.Clear();
Alternatively you can use funcs_to_infer_.Rewind(0) (which is basically an oddly
named "SetLength") to clear the list without deleting the backing buffer.

[Mikhail Naganov, 2009-04-16 16:33:44]

Thanks, Kevin and Lasse! Your comments are really good.

http://codereview.chromium.org/67168/diff/1/2
File src/func-name-inferrer.cc (right):

http://codereview.chromium.org/67168/diff/1/2#newcode68
Line 68: return;
On 2009/04/16 08:06:58, Kevin Millikin wrote:
> MaybeInferFunctionName is only called from InferAndLeave, which is inlined. 
It
> might make sense to move the check and early bailout into InferAndLeave.

Done. I also renamed it to InferFunctionsNames.

http://codereview.chromium.org/67168/diff/1/2#newcode74
Line 74: funcs_to_infer_.Clear();
On 2009/04/16 08:06:58, Kevin Millikin wrote:
> Clearing the list deletes the backing buffer, which then gets reallocated on
the
> first subsequent insertion and resized on the second.  You might consider
> changing the loop to 'while (!funcs_to_infer_.is_empty()) ...' and use
> List::RemoveLast and FuncNameInferrer::set_inferred_name in the body.

I followed Lasse's advice.

http://codereview.chromium.org/67168/diff/1/2#newcode74
Line 74: funcs_to_infer_.Clear();
On 2009/04/16 08:31:23, Lasse Reichstein wrote:
> Alternatively you can use funcs_to_infer_.Rewind(0) (which is basically an
oddly
> named "SetLength") to clear the list without deleting the backing buffer.
> 

Done.

http://codereview.chromium.org/67168/diff/1/3
File src/func-name-inferrer.h (right):

http://codereview.chromium.org/67168/diff/1/3#newcode60
Line 60: void Leave() {
On 2009/04/16 08:06:58, Kevin Millikin wrote:
> I don't think Leave is every called explicitly (only by InferAndLeave?), so
you
> might just inline it there and remove it from the API.

Done.

http://codereview.chromium.org/67168/diff/1/4
File src/rewriter.cc (right):

http://codereview.chromium.org/67168/diff/1/4#newcode285
Line 285: scoped_fni.Enter();
On 2009/04/16 08:06:58, Kevin Millikin wrote:
> There is also a call to ScopedFuncNameInferrer::Enter in
> AstOptimizer::VisitCallRuntime that is currently guarded by checking that the
> expression is a function literal.  Do you want to allow other expression types
> there as well?

Um, that's tricky, unfortunately. Consider the example:

var v = f(function() {})

Would I remove the check from VisitCallRuntime, this will erroneously infer a
name to the function passed as a parameter.

Of course, leaving the check disables inferring a name in this case:

(in the global scope)
var f = 0 ? function() {} : function() {}

It seems that in order to pass both tests, I need to add another state into the
FuncNameInferrer, that will be entered upon visiting the CALL AST node. But as
this all seems quite exotic, I'll better leave it as is for now.