first draft of error checking / reporting API

first draft of error checking / reporting API


BUG=

Committed: https://code.google.com/p/skia/source/detail?r=8566

[humper, 2013-04-08 16:41:09]

Includes a test showing how to use the error checking, and negative radii
checking for rounded rects, so the test has something to check for.

[scroggo, 2013-04-08 19:24:53]

https://codereview.chromium.org/13699004/diff/1/src/core/SkError.cpp
File src/core/SkError.cpp (right):

https://codereview.chromium.org/13699004/diff/1/src/core/SkError.cpp#newcode3
src/core/SkError.cpp:3: * Copyright 2011 Google Inc.
2013

https://codereview.chromium.org/13699004/diff/1/src/core/SkError.cpp#newcode22
src/core/SkError.cpp:22: delete reinterpret_cast<SkError*>(v);
SkDELETE.

Also, shouldn't you reinterpret as an int, since you created it as an int?

https://codereview.chromium.org/13699004/diff/1/src/core/SkError.cpp#newcode31
src/core/SkError.cpp:31: delete reinterpret_cast<SkErrorCallbackFunction *>(v);
SkDELETE_ARRAY

https://codereview.chromium.org/13699004/diff/1/src/core/SkError.cpp#newcode35
src/core/SkError.cpp:35: *(reinterpret_cast<SkErrorCallbackFunction
*>(SkTLS::Get(CreateThreadErrorCallback, DeleteThreadErrorCallback)))
100 chars

https://codereview.chromium.org/13699004/diff/1/src/core/SkError.cpp#newcode41
src/core/SkError.cpp:41: delete reinterpret_cast<void **>(v);
SkDELETE

https://codereview.chromium.org/13699004/diff/1/src/core/SkError.cpp#newcode52
src/core/SkError.cpp:52: delete reinterpret_cast<char *>(v);
SkDELETE_ARRAY

https://codereview.chromium.org/13699004/diff/1/src/core/SkError.cpp#newcode74
src/core/SkError.cpp:74: SkDebugf("Initializing the error system\n");
It seems to me that this adds clutter to the debug output when starting up. Is
this useful generally?

https://codereview.chromium.org/13699004/diff/1/src/core/SkError.cpp#newcode117
src/core/SkError.cpp:117: case kError_ParseError:       error_name = "Parse
Error";       break;
Generally we don't put an entire case on one line, although I'm fine with it if
Mike is.

However, default should be handled according to the style guide.

https://codereview.chromium.org/13699004/diff/1/src/core/SkErrorInternals.h
File src/core/SkErrorInternals.h (right):

https://codereview.chromium.org/13699004/diff/1/src/core/SkErrorInternals.h#n...
src/core/SkErrorInternals.h:15: public:
Minor nit: public should be preceded by a newline.

https://codereview.chromium.org/13699004/diff/1/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

https://codereview.chromium.org/13699004/diff/1/src/core/SkPath.cpp#newcode16
src/core/SkPath.cpp:16: #include "SkErrorInternals.h"
Should be in alphabetical order.

https://codereview.chromium.org/13699004/diff/1/tests/ErrorTest.cpp
File tests/ErrorTest.cpp (right):

https://codereview.chromium.org/13699004/diff/1/tests/ErrorTest.cpp#newcode13
tests/ErrorTest.cpp:13: #define CHECK(errcode) REPORTER_ASSERT( reporter, (err =
SkGetLastError()) == kError_##errcode); \
Minor nit:
I find macros to be more readable if the '\'s line up.

[humper, 2013-04-08 19:43:23]

https://codereview.chromium.org/13699004/diff/1/src/core/SkError.cpp
File src/core/SkError.cpp (right):

https://codereview.chromium.org/13699004/diff/1/src/core/SkError.cpp#newcode3
src/core/SkError.cpp:3: * Copyright 2011 Google Inc.
On 2013/04/08 19:24:53, scroggo wrote:
> 2013

Done.

https://codereview.chromium.org/13699004/diff/1/src/core/SkError.cpp#newcode22
src/core/SkError.cpp:22: delete reinterpret_cast<SkError*>(v);
On 2013/04/08 19:24:53, scroggo wrote:
> SkDELETE.
> 
> Also, shouldn't you reinterpret as an int, since you created it as an int?
> 

Done.

https://codereview.chromium.org/13699004/diff/1/src/core/SkError.cpp#newcode31
src/core/SkError.cpp:31: delete reinterpret_cast<SkErrorCallbackFunction *>(v);
On 2013/04/08 19:24:53, scroggo wrote:
> SkDELETE_ARRAY

Done.

https://codereview.chromium.org/13699004/diff/1/src/core/SkError.cpp#newcode35
src/core/SkError.cpp:35: *(reinterpret_cast<SkErrorCallbackFunction
*>(SkTLS::Get(CreateThreadErrorCallback, DeleteThreadErrorCallback)))
On 2013/04/08 19:24:53, scroggo wrote:
> 100 chars

Done.

https://codereview.chromium.org/13699004/diff/1/src/core/SkError.cpp#newcode41
src/core/SkError.cpp:41: delete reinterpret_cast<void **>(v);
On 2013/04/08 19:24:53, scroggo wrote:
> SkDELETE

Done.

https://codereview.chromium.org/13699004/diff/1/src/core/SkError.cpp#newcode52
src/core/SkError.cpp:52: delete reinterpret_cast<char *>(v);
On 2013/04/08 19:24:53, scroggo wrote:
> SkDELETE_ARRAY

Done.

https://codereview.chromium.org/13699004/diff/1/src/core/SkError.cpp#newcode74
src/core/SkError.cpp:74: SkDebugf("Initializing the error system\n");
On 2013/04/08 19:24:53, scroggo wrote:
> It seems to me that this adds clutter to the debug output when starting up. Is
> this useful generally?

Done.

https://codereview.chromium.org/13699004/diff/1/src/core/SkError.cpp#newcode117
src/core/SkError.cpp:117: case kError_ParseError:       error_name = "Parse
Error";       break;
On 2013/04/08 19:24:53, scroggo wrote:
> Generally we don't put an entire case on one line, although I'm fine with it
if
> Mike is.
> 
> However, default should be handled according to the style guide.

Done.

https://codereview.chromium.org/13699004/diff/1/src/core/SkErrorInternals.h
File src/core/SkErrorInternals.h (right):

https://codereview.chromium.org/13699004/diff/1/src/core/SkErrorInternals.h#n...
src/core/SkErrorInternals.h:15: public:
On 2013/04/08 19:24:53, scroggo wrote:
> Minor nit: public should be preceded by a newline.

Done.

https://codereview.chromium.org/13699004/diff/1/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

https://codereview.chromium.org/13699004/diff/1/src/core/SkPath.cpp#newcode16
src/core/SkPath.cpp:16: #include "SkErrorInternals.h"
On 2013/04/08 19:24:53, scroggo wrote:
> Should be in alphabetical order.

Done.

https://codereview.chromium.org/13699004/diff/1/tests/ErrorTest.cpp
File tests/ErrorTest.cpp (right):

https://codereview.chromium.org/13699004/diff/1/tests/ErrorTest.cpp#newcode13
tests/ErrorTest.cpp:13: #define CHECK(errcode) REPORTER_ASSERT( reporter, (err =
SkGetLastError()) == kError_##errcode); \
On 2013/04/08 19:24:53, scroggo wrote:
> Minor nit:
> I find macros to be more readable if the '\'s line up.

Done.

[reed1, 2013-04-08 19:47:31]

https://codereview.chromium.org/13699004/diff/6001/include/core/SkError.h
File include/core/SkError.h (right):

https://codereview.chromium.org/13699004/diff/6001/include/core/SkError.h#new...
include/core/SkError.h:11: typedef enum {
I think all of our other enums are just

enum SkError {
...
};

https://codereview.chromium.org/13699004/diff/6001/include/core/SkError.h#new...
include/core/SkError.h:11: typedef enum {
Skia's other enums are named in reverse order from these.

e.g.

kNone_SkError,
kInvalidArgument_SkError,
...

https://codereview.chromium.org/13699004/diff/6001/include/core/SkError.h#new...
include/core/SkError.h:15: kError_InvalidHandle,
What is a 'Handle' in the Skia world?

https://codereview.chromium.org/13699004/diff/6001/include/core/SkError.h#new...
include/core/SkError.h:16: kError_InvalidPaint,
What is an invalid paint?

https://codereview.chromium.org/13699004/diff/6001/include/core/SkError.h#new...
include/core/SkError.h:21: SkError SkGetLastError();
Need some dox for each of these, including that they are per-thread.

[reed1, 2013-04-08 19:48:25]

Alternate crazy idea for organizing this...

class SkError {
public:
    enum Type {
        ...
    };

    static Type GetLastError();
    ...
};

[humper, 2013-04-08 20:10:55]

https://codereview.chromium.org/13699004/diff/6001/include/core/SkError.h
File include/core/SkError.h (right):

https://codereview.chromium.org/13699004/diff/6001/include/core/SkError.h#new...
include/core/SkError.h:11: typedef enum {
Okay, I'll turn these around.

On 2013/04/08 19:47:31, reed1 wrote:
> Skia's other enums are named in reverse order from these.
> 
> e.g.
> 
> kNone_SkError,
> kInvalidArgument_SkError,
> ...

https://codereview.chromium.org/13699004/diff/6001/include/core/SkError.h#new...
include/core/SkError.h:15: kError_InvalidHandle,
On 2013/04/08 19:47:31, reed1 wrote:
> What is a 'Handle' in the Skia world?

Done.

https://codereview.chromium.org/13699004/diff/6001/include/core/SkError.h#new...
include/core/SkError.h:21: SkError SkGetLastError();
On 2013/04/08 19:47:31, reed1 wrote:
> Need some dox for each of these, including that they are per-thread.

Done.

[scroggo, 2013-04-08 20:38:42]

lgtm

https://codereview.chromium.org/13699004/diff/10001/include/core/SkError.h
File include/core/SkError.h (right):

https://codereview.chromium.org/13699004/diff/10001/include/core/SkError.h#ne...
include/core/SkError.h:70: *  \param cb The callback function to be invoked. 
Passing NULL
Minor nit:

Most of our parameters in Skia use "@param" instead of "\param".

Nit for the style-guide:
It only states that we use doxygen-style docs, which is fairly open-ended.

[reed1, 2013-04-08 20:49:57]

Can we make any TLS stuff be lazily initialized, so we don't have to require
Init()?

[humper, 2013-04-08 20:58:36]

On 2013/04/08 20:49:57, reed1 wrote:
> Can we make any TLS stuff be lazily initialized, so we don't have to require
> Init()?

Yep, latest patch removes the requirement for the Init() call.  Have a looksee

[scroggo, 2013-04-08 21:01:34]

https://codereview.chromium.org/13699004/diff/1028/src/core/SkError.cpp
File src/core/SkError.cpp (right):

https://codereview.chromium.org/13699004/diff/1028/src/core/SkError.cpp#newco...
src/core/SkError.cpp:31: SkDELETE_ARRAY(reinterpret_cast<SkErrorCallbackFunction
*>(v));
Should now be SkDELETE, since an array is no longer created.

https://codereview.chromium.org/13699004/diff/1028/src/core/SkGraphics.cpp
File src/core/SkGraphics.cpp (right):

https://codereview.chromium.org/13699004/diff/1028/src/core/SkGraphics.cpp#ne...
src/core/SkGraphics.cpp:31: #include "SkErrorInternals.h"
No longer needed.

[humper, 2013-04-08 21:15:18]

https://codereview.chromium.org/13699004/diff/1028/src/core/SkError.cpp
File src/core/SkError.cpp (right):

https://codereview.chromium.org/13699004/diff/1028/src/core/SkError.cpp#newco...
src/core/SkError.cpp:31: SkDELETE_ARRAY(reinterpret_cast<SkErrorCallbackFunction
*>(v));
On 2013/04/08 21:01:34, scroggo wrote:
> Should now be SkDELETE, since an array is no longer created.

Done.

https://codereview.chromium.org/13699004/diff/1028/src/core/SkGraphics.cpp
File src/core/SkGraphics.cpp (right):

https://codereview.chromium.org/13699004/diff/1028/src/core/SkGraphics.cpp#ne...
src/core/SkGraphics.cpp:31: #include "SkErrorInternals.h"
On 2013/04/08 21:01:34, scroggo wrote:
> No longer needed.

Done.

[scroggo, 2013-04-08 21:23:26]

lgtm

[reed1, 2013-04-08 21:26:03]

lgtm

[commit-bot: I haz the power, 2013-04-08 21:33:00]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/humper@google.com/13699004/3004

[humper, 2013-04-08 21:44:20]

Committed patchset #6 manually as r8566 (presubmit successful).