AstValueFactory: make true, false, null, undefined and "the hole" unique values.

AstValueFactory: make true, false, null, undefined and "the hole" unique values.

They were not, so we were creating several instances of them, one for each time
they occurred in the source code.

It's not known to have caused efficiency problems though, so this is a sanity
fix more than an efficiency fix.

Note that numbers are still not unique.

BUG=
R=rossberg@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=24873

[marja, 2014-10-23 11:17:38]

marja@chromium.org changed reviewers:
+ rossberg@chromium.org

[marja, 2014-10-23 11:17:39]

rossberg, ptal

[rossberg, 2014-10-23 12:12:22]

https://codereview.chromium.org/657893003/diff/1/src/ast-value-factory.h
File src/ast-value-factory.h (right):

https://codereview.chromium.org/657893003/diff/1/src/ast-value-factory.h#newc...
src/ast-value-factory.h:289: #define F(name, value) name##_ = NULL;
Nit: use same formatting as above.

https://codereview.chromium.org/657893003/diff/1/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/657893003/diff/1/src/ast.h#newcode3404
src/ast.h:3404: const AstValue* value = b ? ast_value_factory_->true_value()
Instead of making these changes here, can't we encapsulate the caching by having
the respective New operators just return the cached versions internally?

[marja, 2014-10-23 12:27:32]

https://codereview.chromium.org/657893003/diff/1/src/ast-value-factory.h
File src/ast-value-factory.h (right):

https://codereview.chromium.org/657893003/diff/1/src/ast-value-factory.h#newc...
src/ast-value-factory.h:289: #define F(name, value) name##_ = NULL;
On 2014/10/23 12:12:21, rossberg wrote:
> Nit: use same formatting as above.

This was doings of clang-format, so changed the above version to what
clang-format wants, too.

https://codereview.chromium.org/657893003/diff/1/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/657893003/diff/1/src/ast.h#newcode3404
src/ast.h:3404: const AstValue* value = b ? ast_value_factory_->true_value()
On 2014/10/23 12:12:21, rossberg wrote:
> Instead of making these changes here, can't we encapsulate the caching by
having
> the respective New operators just return the cached versions internally?

Do you mean only for booleans (true_value & false_value), or for all changes in
this file (undefined_value instead of NewUndefined)?

I think this version is clearer, since it makes it clear when new stuff is
created (NewSomething, such as NewConsString, returns a new object, things like
null_value() and this_string() return pointers to cached objects).

[rossberg, 2014-10-23 12:36:31]

https://codereview.chromium.org/657893003/diff/1/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/657893003/diff/1/src/ast.h#newcode3404
src/ast.h:3404: const AstValue* value = b ? ast_value_factory_->true_value()
On 2014/10/23 12:27:32, marja wrote:
> On 2014/10/23 12:12:21, rossberg wrote:
> > Instead of making these changes here, can't we encapsulate the caching by
> having
> > the respective New operators just return the cached versions internally?
> 
> Do you mean only for booleans (true_value & false_value), or for all changes
in
> this file (undefined_value instead of NewUndefined)?
> 
> I think this version is clearer, since it makes it clear when new stuff is
> created (NewSomething, such as NewConsString, returns a new object, things
like
> null_value() and this_string() return pointers to cached objects).

For all. It's preferable because it properly abstracts caching as an
implementation detail instead of leaking it into the interface. That's more
uniform and more extensible (e.g., one might implement caching of some common
number literals as well). The calling code shouldn't need to care.

[marja, 2014-10-23 13:00:41]

https://codereview.chromium.org/657893003/diff/1/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/657893003/diff/1/src/ast.h#newcode3404
src/ast.h:3404: const AstValue* value = b ? ast_value_factory_->true_value()
On 2014/10/23 12:36:31, rossberg wrote:
> On 2014/10/23 12:27:32, marja wrote:
> > On 2014/10/23 12:12:21, rossberg wrote:
> > > Instead of making these changes here, can't we encapsulate the caching by
> > having
> > > the respective New operators just return the cached versions internally?
> > 
> > Do you mean only for booleans (true_value & false_value), or for all changes
> in
> > this file (undefined_value instead of NewUndefined)?
> > 
> > I think this version is clearer, since it makes it clear when new stuff is
> > created (NewSomething, such as NewConsString, returns a new object, things
> like
> > null_value() and this_string() return pointers to cached objects).
> 
> For all. It's preferable because it properly abstracts caching as an
> implementation detail instead of leaking it into the interface. That's more
> uniform and more extensible (e.g., one might implement caching of some common
> number literals as well). The calling code shouldn't need to care.

Fair enough. Done.

[rossberg, 2014-10-23 13:21:29]

Sorry, more wishes. :)

https://codereview.chromium.org/657893003/diff/40001/src/ast-value-factory.h
File src/ast-value-factory.h (right):

https://codereview.chromium.org/657893003/diff/40001/src/ast-value-factory.h#...
src/ast-value-factory.h:323: #define F(name, function_name, type)        \
Hm, this second-order macro magic still makes it rather hard to see what's being
declared here. TBH, it seems overkill for a list of 3 functions that is unlikely
to ever be extended.

Can we have these functions declared explicitly, as before, and only use a
(first-order) macro for their implementation in the .cc file?

[marja, 2014-10-24 10:08:19]

https://codereview.chromium.org/657893003/diff/40001/src/ast-value-factory.h
File src/ast-value-factory.h (right):

https://codereview.chromium.org/657893003/diff/40001/src/ast-value-factory.h#...
src/ast-value-factory.h:323: #define F(name, function_name, type)        \
On 2014/10/23 13:21:29, rossberg wrote:
> Hm, this second-order macro magic still makes it rather hard to see what's
being
> declared here. TBH, it seems overkill for a list of 3 functions that is
unlikely
> to ever be extended.
> 
> Can we have these functions declared explicitly, as before, and only use a
> (first-order) macro for their implementation in the .cc file?

Tbh, I'm not sure what kind of macro you meant for generating the
implementation, but I took a guess - please have a look. It's questionable
though whether that macro adds any value over writing the implementations by
hand.

[rossberg, 2014-10-24 10:40:05]

LGTM

https://codereview.chromium.org/657893003/diff/40001/src/ast-value-factory.h
File src/ast-value-factory.h (right):

https://codereview.chromium.org/657893003/diff/40001/src/ast-value-factory.h#...
src/ast-value-factory.h:323: #define F(name, function_name, type)        \
On 2014/10/24 10:08:19, marja wrote:
> On 2014/10/23 13:21:29, rossberg wrote:
> > Hm, this second-order macro magic still makes it rather hard to see what's
> being
> > declared here. TBH, it seems overkill for a list of 3 functions that is
> unlikely
> > to ever be extended.
> > 
> > Can we have these functions declared explicitly, as before, and only use a
> > (first-order) macro for their implementation in the .cc file?
> 
> Tbh, I'm not sure what kind of macro you meant for generating the
> implementation, but I took a guess - please have a look. It's questionable
> though whether that macro adds any value over writing the implementations by
> hand.

More or less what you had as F here (or similarly your GENERATE_VALUE_GETTER
now). I'm fine with reverting that bit of macrofication, too, but I assumed
you'd prefer to keep it. :)

[marja, 2014-10-24 13:02:52]

Committed patchset #6 (id:100001) manually as 24873 (presubmit successful).