On 2010/05/10 06:25:57, Søren Gjesse wrote:
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.
omitted -> Committed
http://codereview.chromium.org/1985004/show
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
