On 2010/05/07 15:45:40, jaimeyap wrote:
http://codereview.chromium.org/1985004/diff/2002/9002
File src/top.cc (right):
http://codereview.chromium.org/1985004/diff/2002/9002#newcode422
src/top.cc:422: if (fun_name->ToBoolean()->IsFalse()) {
On 2010/05/07 15:25:27, Michail Naganov wrote:
> On 2010/05/07 14:49:42, jaimeyap wrote:
> > On 2010/05/07 11:39:52, Michail Naganov wrote:
> > > On 2010/05/07 07:57:06, Søren Gjesse wrote:
> > > > Any reason for changing this?
> > > >
> > > > I just looked at the code, and we have at least three ways of
checking
> this
> > > >
> > > > if (!fun_name->IsString())
> > > > if (fun_name->IsUndefined())
> > > > if (fun_name->ToBoolean()->IsFalse())
> > > >
> > > > I think we should be consistent, so I suggest adding a function
called
> > > something
> > > > likeGetNameOrInferredName/GetDisplayName/GetFriendlyName to
JSFunction
and
> > use
> > > > that in all places where the inferred name is returned if no
actual
name
> is
> > > > found.
> > >
> > > I can do this as a separate change. But I'm too not quite
understanding,
why
> > use
> > > such extravagant "ToBoolean()->IsFalse()" check. Using
> > > "shared->name()->IsUndefined()" is more straightforward.
> >
> >
> > Because it is not undefined. name and inferred name get set to the
empty
> string
> > initially :(. It tripped me up too.
> >
> > The empty string in javascript boolean coerces to false. In fact, any
> whitespace
> > only string boolean coerces to false.
> >
> > ("" == false) == true;
> > ("\r\n" == false) == true;
> > (" " == false) == true;
> >
> > Gross eh?
> >
> > Also, I agree with Michail that we should normalize all the getters
for a
> > function name in a separate change. This one should go in soon since
it
fixed
> > API brokenness.
>
> Yeah, I knew that. I'm sorry, I looked into somebody's wrong code which
checks
> for undefined. Indeed, function's name is usually a string and it's
empty if
> it's anonymous. This is another point for introducing a function that
returns
a
> "friendly" name -- there is already a piece of code that seem to do it
in a
> wrong way (Accessors::ScriptGetEvalFromFunctionName). I meant that
converting
a
> string to boolean is an overkill, since parser already does a good job
in
> squeezing whitespaces, so function's name can't be e.g. " " (two
spaces),
it
> can only have zero length, if it's anonymous.
Im going to go with the principle of least surprise. If in JS I write:
var name = fun.name() || fun.inferred_name();
It is doing a proper boolean coercion. I would hate to be sensitive to a
change
down the line in the default for name that boolean coerces but isn't
exactly
the
emtpy string :).
In any case, I think we should figure out the correct semantics
for "friendly
name" when we normalize all the callers to use this shared getter (in a
subsequent patch).
ommitted. Closing issue.
http://codereview.chromium.org/1985004/show
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
