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

Issue 11648006: Create read only handles for empty_array and sentinel objects (Closed)

Created:
8 years ago by siva
Modified:
8 years ago
Reviewers:
Ivan Posva
CC:
reviews_dartlang.org, vm-dev_dartlang.org
Visibility:
Public.

Description

Create read only handles for empty_array and sentinel objects (trying out a basic framework and will extend it to others once this works). Committed: https://code.google.com/p/dart/source/detail?r=16416

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Total comments: 9

Patch Set 4 : #

Patch Set 5 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+181 lines, -173 lines) Patch
M lib/array.cc View 1 2 3 1 chunk +1 line, -2 lines 0 comments Download
M lib/isolate.cc View 1 2 3 1 chunk +1 line, -2 lines 0 comments Download
M lib/string.cc View 1 2 3 1 chunk +1 line, -2 lines 0 comments Download
M vm/ast.cc View 1 2 3 1 chunk +2 lines, -3 lines 0 comments Download
M vm/class_finalizer_test.cc View 1 2 3 1 chunk +3 lines, -4 lines 0 comments Download
M vm/code_descriptors.cc View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M vm/code_descriptors_test.cc View 1 2 3 1 chunk +2 lines, -3 lines 0 comments Download
M vm/code_generator_test.cc View 1 2 3 1 chunk +2 lines, -3 lines 0 comments Download
M vm/compiler.cc View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download
M vm/dart.h View 1 2 3 2 chunks +5 lines, -0 lines 0 comments Download
M vm/dart.cc View 1 2 3 4 chunks +38 lines, -0 lines 0 comments Download
M vm/dart_api_impl.cc View 1 2 3 3 chunks +6 lines, -6 lines 0 comments Download
M vm/dart_entry_test.cc View 1 2 3 2 chunks +4 lines, -6 lines 0 comments Download
M vm/debugger.cc View 1 2 3 2 chunks +3 lines, -4 lines 0 comments Download
M vm/exceptions.cc View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download
M vm/flow_graph_optimizer.h View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M vm/flow_graph_optimizer.cc View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download
M vm/handles.h View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
vm/intermediate_language.cc View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download
M vm/object.h View 1 2 3 5 chunks +21 lines, -7 lines 0 comments Download
M vm/object.cc View 1 2 3 19 chunks +50 lines, -47 lines 0 comments Download
M vm/object_store.cc View 1 2 3 1 chunk +3 lines, -3 lines 0 comments Download
M vm/object_test.cc View 1 2 3 3 chunks +3 lines, -5 lines 0 comments Download
M vm/parser.cc View 1 2 3 10 chunks +14 lines, -17 lines 0 comments Download
M vm/snapshot.cc View 1 2 3 2 chunks +4 lines, -4 lines 0 comments Download
M vm/stub_code_ia32_test.cc View 1 2 3 2 chunks +2 lines, -4 lines 0 comments Download
M vm/stub_code_x64_test.cc View 1 2 3 2 chunks +2 lines, -4 lines 0 comments Download
M vm/symbols.h View 1 2 3 2 chunks +0 lines, -23 lines 0 comments Download
M vm/symbols.cc View 1 2 3 3 chunks +1 line, -9 lines 0 comments Download
M vm/unit_test.h View 1 2 3 2 chunks +2 lines, -4 lines 0 comments Download

Messages

Total messages: 4 (0 generated)
siva
8 years ago (2012-12-19 18:41:36 UTC) #1
Ivan Posva
LGTM with mostly comments about comments. -Ivan https://codereview.chromium.org/11648006/diff/7005/vm/dart.cc File vm/dart.cc (right): https://codereview.chromium.org/11648006/diff/7005/vm/dart.cc#newcode58 vm/dart.cc:58: // (e.g: ...
8 years ago (2012-12-20 23:43:52 UTC) #2
siva
https://codereview.chromium.org/11648006/diff/7005/vm/dart.cc File vm/dart.cc (right): https://codereview.chromium.org/11648006/diff/7005/vm/dart.cc#newcode58 vm/dart.cc:58: // (e.g: symbols, null object, empty array etc. On ...
8 years ago (2012-12-21 02:28:06 UTC) #3
Ivan Posva
8 years ago (2012-12-21 07:33:41 UTC) #4
Message was sent while issue was closed.
https://codereview.chromium.org/11648006/diff/7005/vm/object.cc
File vm/object.cc (right):

https://codereview.chromium.org/11648006/diff/7005/vm/object.cc#newcode1260
vm/object.cc:1260: Dart::IsReadOnlyHandle(reinterpret_cast<uword>(this)));
On 2012/12/21 02:28:06, siva wrote:
> Added IsReadOnlyHandle method to Object.
> 
> Regarding IsNotTemporaryScopedHandle(): Srdjan was not
> in favour of changing all the places where we have
> ASSERt(IsZoneHandle()) to also be 
> ASSERT(IsZoneHandle() || IsReadOnlyHandle());
> Maybe I should talk with him regarding this and replace
> IsNotTemporaryScopedHandle to something more meaningful.
> 
> On 2012/12/20 23:43:52, Ivan Posva wrote:
> > Could we have a IsReadOnlyHandle similar to IsZoneHandle on object instead?
> > 
> > Also I find it a bit strange to have a method called IsNotXYZ.
> 

I find the name confusing on two fronts:
- The Not in IsNotXYZ. We try to avoid that in general.
- What is a non-temporary scoped handle? Or are all scoped handles temporary,
then either the scoped or the temporary are redundant.

Personally I would favor IsScopedHandle or something less mind-twisting.

Powered by Google App Engine
This is Rietveld 408576698