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

Issue 10014002: Rename Api::NewLocalHandle -> Api::NewHandle. (Closed)

Created:
8 years, 8 months ago by turnidge
Modified:
8 years, 8 months ago
Reviewers:
cshapiro
CC:
reviews_dartlang.org, vm-dev_dartlang.org
Visibility:
Public.

Description

Rename Api::NewLocalHandle -> Api::NewHandle. Api::NewHandle now takes a RawObject* intead of an Object&. - This makes the code shorter and easier to read. - This allows us to drop the DARTSCOPE from Dart_GetNativeArgument. (6% speedup on Benchmark_UseDartApi) Committed: https://code.google.com/p/dart/source/detail?r=6283

Patch Set 1 #

Total comments: 2

Patch Set 2 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+122 lines, -197 lines) Patch
M runtime/vm/dart_api_impl.h View 1 chunk +1 line, -1 line 0 comments Download
M runtime/vm/dart_api_impl.cc View 59 chunks +72 lines, -125 lines 0 comments Download
M runtime/vm/dart_api_impl_test.cc View 11 chunks +34 lines, -46 lines 0 comments Download
M runtime/vm/dart_api_state.h View 1 chunk +1 line, -1 line 0 comments Download
M runtime/vm/debugger_api_impl.cc View 9 chunks +10 lines, -20 lines 0 comments Download
M runtime/vm/native_entry.cc View 1 chunk +1 line, -1 line 0 comments Download
M runtime/vm/parser.cc View 1 chunk +3 lines, -3 lines 0 comments Download

Messages

Total messages: 3 (0 generated)
turnidge
8 years, 8 months ago (2012-04-05 22:27:37 UTC) #1
cshapiro
lgtm - much cleaner! https://chromiumcodereview.appspot.com/10014002/diff/1/runtime/vm/dart_api_impl.cc File runtime/vm/dart_api_impl.cc (right): https://chromiumcodereview.appspot.com/10014002/diff/1/runtime/vm/dart_api_impl.cc#newcode388 runtime/vm/dart_api_impl.cc:388: } else if (obj.IsInstance()) { ...
8 years, 8 months ago (2012-04-05 23:23:24 UTC) #2
turnidge
8 years, 8 months ago (2012-04-06 16:50:48 UTC) #3
https://chromiumcodereview.appspot.com/10014002/diff/1/runtime/vm/dart_api_im...
File runtime/vm/dart_api_impl.cc (right):

https://chromiumcodereview.appspot.com/10014002/diff/1/runtime/vm/dart_api_im...
runtime/vm/dart_api_impl.cc:388: } else if (obj.IsInstance()) {
I prefer using the pattern you mention when there is asymmetry between the
cases.  Perhaps error bailouts followed by common case, or vice-versa, or maybe
fast-path(s) followed by slow-path.  In a case like this where I think of all
three clauses being parallel, I prefer to keep them parallel in the code.

On 2012/04/05 23:23:24, cshapiro wrote:
> When I have made a similar change, I was encourage to write my code like
this...
> 
>   if (...) {
>     return ...
>   }
>   if (...) {
>     return ...
>   }
>   return ...
> 
> I think the desire was to lose the trailing else with a return and nothing
> afterward.

Powered by Google App Engine
This is Rietveld 408576698