Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(126)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 9475021: Refactor Constants. (Closed)

Created:
8 years, 10 months ago by floitsch
Modified:
8 years, 9 months ago
CC:
reviews_dartlang.org
Visibility:
Public.

Description

Patch Set 1 #

Total comments: 9

Patch Set 2 : cosmetic changes. #

Total comments: 28

Patch Set 3 : Address comments. #

Patch Set 4 : Fix typo. #

Patch Set 5 : Address comment. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+488 lines, -148 lines) Patch
M frog/leg/compile_time_constants.dart View 1 2 3 4 11 chunks +467 lines, -147 lines 0 comments Download
M frog/leg/ssa/builder.dart View 1 2 1 chunk +1 line, -1 line 0 comments Download
M tests/language/language.status View 1 chunk +1 line, -0 lines 0 comments Download
M tests/language/language-leg.status View 1 2 1 chunk +1 line, -0 lines 0 comments Download
A tests/language/src/CompileTimeConstant8Test.dart View 1 chunk +18 lines, -0 lines 0 comments Download

Messages

Total messages: 6 (0 generated)
floitsch
Normally there should be no new features. This is just a refactoring so that we ...
8 years, 10 months ago (2012-02-27 20:24:58 UTC) #1
floitsch
8 years, 10 months ago (2012-02-27 20:25:14 UTC) #2
kasperl
LGTM as a first step. Do you plan on cleaning up the folding code in ...
8 years, 9 months ago (2012-02-28 09:16:34 UTC) #3
ngeoffray
LGTM https://chromiumcodereview.appspot.com/9475021/diff/5002/frog/leg/compile_time_constants.dart File frog/leg/compile_time_constants.dart (right): https://chromiumcodereview.appspot.com/9475021/diff/5002/frog/leg/compile_time_constants.dart#newcode7 frog/leg/compile_time_constants.dart:7: I would put extra spaces between things that ...
8 years, 9 months ago (2012-02-28 09:46:12 UTC) #4
karlklose
LGTM. https://chromiumcodereview.appspot.com/9475021/diff/5002/frog/leg/compile_time_constants.dart File frog/leg/compile_time_constants.dart (right): https://chromiumcodereview.appspot.com/9475021/diff/5002/frog/leg/compile_time_constants.dart#newcode20 frog/leg/compile_time_constants.dart:20: bool isUser() => false; Can you find a ...
8 years, 9 months ago (2012-02-28 12:46:25 UTC) #5
floitsch
8 years, 9 months ago (2012-02-28 13:13:00 UTC) #6 
https://chromiumcodereview.appspot.com/9475021/diff/1/frog/leg/compile_time_c...
File frog/leg/compile_time_constants.dart (right):

https://chromiumcodereview.appspot.com/9475021/diff/1/frog/leg/compile_time_c...
frog/leg/compile_time_constants.dart:140: bool operator ==(var other) {
On 2012/02/28 09:16:34, kasperl wrote:
> No hashCode?

hashCode is covered by PrimitiveConstant (return value.hashCode()).
Moved it down to the subclasses.

https://chromiumcodereview.appspot.com/9475021/diff/1/frog/leg/compile_time_c...
frog/leg/compile_time_constants.dart:194: bool operator ==(var other) {
On 2012/02/28 09:16:34, kasperl wrote:
> No hashCode?

Done in PrimitiveConstant.

https://chromiumcodereview.appspot.com/9475021/diff/1/frog/leg/compile_time_c...
frog/leg/compile_time_constants.dart:237: // TODO(floitsch): implement real
hash, or delegate to DartString.
On 2012/02/28 09:16:34, kasperl wrote:
> Why not start out by using value.toString().hashCode()?

Done.

https://chromiumcodereview.appspot.com/9475021/diff/1/frog/leg/compile_time_c...
frog/leg/compile_time_constants.dart:414: if (!result.isObject())
On 2012/02/28 09:16:34, kasperl wrote:
> Use { } for multiline ifs.

that was an accident. Should have been on one line.
done.

https://chromiumcodereview.appspot.com/9475021/diff/5002/frog/leg/compile_tim...
File frog/leg/compile_time_constants.dart (right):

https://chromiumcodereview.appspot.com/9475021/diff/5002/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:7: 
On 2012/02/28 09:46:12, ngeoffray wrote:
> I would put extra spaces between things that should be overridden by
subclasses
> (isNull, isInt, ...), and things that should not (isNum, isObject)

done.

https://chromiumcodereview.appspot.com/9475021/diff/5002/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:20: bool isUser() => false;
On 2012/02/28 12:46:25, karlklose wrote:
> Can you find a better name? Perhaps isNonLiteral or isConstructedObject?

Done.

https://chromiumcodereview.appspot.com/9475021/diff/5002/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:21: bool isObject() => isList() || isMap();
On 2012/02/28 09:46:12, ngeoffray wrote:
> || isUser()?

Done.

https://chromiumcodereview.appspot.com/9475021/diff/5002/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:35: return new BoolConstant(this == other);
On 2012/02/28 09:46:12, ngeoffray wrote:
> Shouldn't those be canonicalized?

The BoolConstant constructor will be a factory that caches the values. For
simplicity not done in this CL.

https://chromiumcodereview.appspot.com/9475021/diff/5002/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:69: int hashCode() => 142341;
On 2012/02/28 09:46:12, ngeoffray wrote:
> Please comment on that magic value.

Done.

https://chromiumcodereview.appspot.com/9475021/diff/5002/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:134: // Visit super in case the [op] was
"==", "===", "!=" or "!===".
On 2012/02/28 09:46:12, ngeoffray wrote:
> -> !==

Done.

https://chromiumcodereview.appspot.com/9475021/diff/5002/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:139: // treated as separate constants.
On 2012/02/28 09:46:12, ngeoffray wrote:
> Please add: 'The is !IntConstant check at the beginning of this function makes
> sure we won't compare ints and doubles.

Done.

https://chromiumcodereview.appspot.com/9475021/diff/5002/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:228: int hashCode() => value ? 499 : 42;
On 2012/02/28 09:46:12, ngeoffray wrote:
> comments please

Done.

https://chromiumcodereview.appspot.com/9475021/diff/5002/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:266: return value == otherString.value;
On 2012/02/28 09:46:12, ngeoffray wrote:
> Check hashCode first?

Done.

https://chromiumcodereview.appspot.com/9475021/diff/5002/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:276: bool isObject() => true;
On 2012/02/28 09:46:12, ngeoffray wrote:
> I think you should remove that one.

Done.

https://chromiumcodereview.appspot.com/9475021/diff/5002/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:279: class ListConstant extends
ObjectConstant {
On 2012/02/28 09:46:12, ngeoffray wrote:
> Missing isList in this class.

Done.

https://chromiumcodereview.appspot.com/9475021/diff/5002/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:289: int hashCode() => _hashCode;
On 2012/02/28 09:46:12, ngeoffray wrote:
> Consistency: I would put this method at the end of the class, just like the
> other classes.

Done.

https://chromiumcodereview.appspot.com/9475021/diff/5002/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:322: class ConstructedConstant extends
ObjectConstant {
On 2012/02/28 09:46:12, ngeoffray wrote:
> Missing isUser in this class.

Done.

https://chromiumcodereview.appspot.com/9475021/diff/5002/frog/leg/compile_tim...
frog/leg/compile_time_constants.dart:336: int hashCode() => _hashCode;
On 2012/02/28 09:46:12, ngeoffray wrote:
> ditto.

Done.

Powered by Google App Engine
This is Rietveld 408576698