Add DART_CHECKVALID

DART_CHECK_VALID -- abort on an invalid handle.
EXPECT_VALID -- check handle validity in unit tests.
Committed: https://code.google.com/p/dart/source/detail?r=970

[siva, 2011-10-27 21:15:54]

LGTM once the issue of ASSERT Vs EXPECT that DynamicAssertionHelper::Fail uses
to decide whether to abort or continue is preserved.

http://codereview.chromium.org/8401010/diff/2002/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

http://codereview.chromium.org/8401010/diff/2002/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:90: const char* error);
I am assuming you want the external users of this API
to also use the DART_CHECKVALID macro? In that case this
would not be an internal routine right.
Also external users may want to control whether the process aborts or does
something else through the macro.

http://codereview.chromium.org/8401010/diff/2002/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:97: }
Not sure what the style should be here, indented with if or over here? We have
been following indent with if.

http://codereview.chromium.org/8401010/diff/2002/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/8401010/diff/2002/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:29: 
extra new line?

http://codereview.chromium.org/8401010/diff/2002/runtime/vm/dart_api_impl_tes...
File runtime/vm/dart_api_impl_test.cc (right):

http://codereview.chromium.org/8401010/diff/2002/runtime/vm/dart_api_impl_tes...
runtime/vm/dart_api_impl_test.cc:15: 
extra new line?

http://codereview.chromium.org/8401010/diff/2002/runtime/vm/dart_api_impl_tes...
runtime/vm/dart_api_impl_test.cc:29: DART_CHECKVALID(result);
This changes the behaviour we have currently as a failure here will abort the
process and not continue with the remaining unit tests when run_vm_tests is
used.

http://codereview.chromium.org/8401010/diff/2002/runtime/vm/dart_api_impl_tes...
runtime/vm/dart_api_impl_test.cc:1512: ASSERT(!Dart_IsValid(result));
I think this should be EXPECT(!Dart_IsValid(result));

http://codereview.chromium.org/8401010/diff/2002/runtime/vm/dart_api_impl_tes...
runtime/vm/dart_api_impl_test.cc:1604: ASSERT(!Dart_IsValid(result));
I think this should be EXPECT(!Dart_IsValid(result));

http://codereview.chromium.org/8401010/diff/2002/runtime/vm/dart_api_impl_tes...
runtime/vm/dart_api_impl_test.cc:1686: ASSERT(!Dart_IsValid(result));
I think this should be EXPECT(!Dart_IsValid(result));

[turnidge, 2011-10-27 21:55:20]

I decided to make a macro called EXPECT_VALID which behaves correctly for our
unit testing framework and keep the DART_CHECK_VALID macro for external users.

http://codereview.chromium.org/8401010/diff/2002/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

http://codereview.chromium.org/8401010/diff/2002/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:90: const char* error);
On 2011/10/27 21:15:54, asiva wrote:
> I am assuming you want the external users of this API
> to also use the DART_CHECKVALID macro? In that case this
> would not be an internal routine right.
> Also external users may want to control whether the process aborts or does
> something else through the macro.

I don't want external users to call _Dart_ReportInvalidHandle directly.  That's
what I mean by internal -- is that the wrong meaning?

I don't want to give external users a non-fatal version.  If they had that, it
would mainly just spam their stderr but not do anything useful for them.  I have
dropped the "fatal" parameter.

http://codereview.chromium.org/8401010/diff/2002/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:93: #define DART_CHECKVALID(handle)                  
                      \
Which do you like better... DART_CHECKVALID or DART_CHECK_VALID.  I've decided
to go with the latter.

http://codereview.chromium.org/8401010/diff/2002/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:97: }
On 2011/10/27 21:15:54, asiva wrote:
> Not sure what the style should be here, indented with if or over here? We have
> been following indent with if.

Fixed.

http://codereview.chromium.org/8401010/diff/2002/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/8401010/diff/2002/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:29: 
On 2011/10/27 21:15:54, asiva wrote:
> extra new line?

Fixed.

http://codereview.chromium.org/8401010/diff/2002/runtime/vm/dart_api_impl_tes...
File runtime/vm/dart_api_impl_test.cc (right):

http://codereview.chromium.org/8401010/diff/2002/runtime/vm/dart_api_impl_tes...
runtime/vm/dart_api_impl_test.cc:15: 
On 2011/10/27 21:15:54, asiva wrote:
> extra new line?

Fixed.

http://codereview.chromium.org/8401010/diff/2002/runtime/vm/dart_api_impl_tes...
runtime/vm/dart_api_impl_test.cc:29: DART_CHECKVALID(result);
On 2011/10/27 21:15:54, asiva wrote:
> This changes the behaviour we have currently as a failure here will abort the
> process and not continue with the remaining unit tests when run_vm_tests is
> used.

Added a separate EXPECT_VALID which I will use in tests.  Now DART_CHECK_VALID
is mainly for external users.

http://codereview.chromium.org/8401010/diff/2002/runtime/vm/dart_api_impl_tes...
runtime/vm/dart_api_impl_test.cc:1512: ASSERT(!Dart_IsValid(result));
On 2011/10/27 21:15:54, asiva wrote:
> I think this should be EXPECT(!Dart_IsValid(result));

Done, here and 2x below.

[siva, 2011-10-31 19:49:22]

lgtm

http://codereview.chromium.org/8401010/diff/11001/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/8401010/diff/11001/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:47: OS::Abort();
If we expect embedding code to use this then OS::Abort might be a bit harsh. I
am thinking of the scenario where some server is embedding the VM and abort
might not be the right thing.
I think this is ok for now but we could consider making this as something only
the VM uses.

[turnidge, 2011-10-31 21:30:22]

http://codereview.chromium.org/8401010/diff/11001/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/8401010/diff/11001/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:47: OS::Abort();
On 2011/10/31 19:49:22, asiva wrote:
> If we expect embedding code to use this then OS::Abort might be a bit harsh. I
> am thinking of the scenario where some server is embedding the VM and abort
> might not be the right thing.
> I think this is ok for now but we could consider making this as something only
> the VM use

My intention is that this is a convenience function for those who know that they
want to abort.  Those that do not want to abort will use Dart_IsValid and try to
recover gracefully.