Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added ability for callbacks to have time.Time as arguments #604

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 46 additions & 2 deletions callback.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,12 @@ import (
"math"
"reflect"
"sync"
"time"
"unsafe"
)

var timetype = reflect.TypeOf(time.Now())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should just do reflect.TypeOf(time.Time{}). No need to take the hit of actually fetching the system times at startup.


//export callbackTrampoline
func callbackTrampoline(ctx *C.sqlite3_context, argc int, argv **C.sqlite3_value) {
args := (*[(math.MaxInt32 - 1) / unsafe.Sizeof((*C.sqlite3_value)(nil))]*C.sqlite3_value)(unsafe.Pointer(argv))[:argc:argc]
Expand Down Expand Up @@ -243,10 +246,40 @@ func callbackArg(typ reflect.Type) (callbackArgConverter, error) {
c := callbackArgCast{callbackArgFloat64, typ}
return c.Run, nil
default:
return nil, fmt.Errorf("don't know how to convert to %s", typ)
switch typ {
case timetype:
Copy link
Collaborator

@rittneje rittneje Oct 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is possible that the callback arg is not time.Time directly, but rather a typedef on it (e.g., type Foo time.Time). If we want to support such things, then instead you want to check if time.Time is convertible to typ (using the ConvertibleTo method).

On that note, it looks like there are actually bugs in the existing logic on this point. For instance, if I have a callback that takes a string typedef (e.g., type Bar string), that will be allowed, because the kind of that type is still reflect.String. But later the call will lead to a runtime panic. https://play.golang.org/p/MI-324y6veS

Fixing that will require appropriate calls to Convert. For example: https://play.golang.org/p/k77Y82MDNeL

The problem is even more interesting for []byte (aka []uint8), because you cannot actually convert between []byte and []TypedefOnByte. That particular case should probably be fixed to just check if the arg kind is not string and []byte is convertible to the arg type (or the arg type is specifically []byte) rather than doing a kind-based check.

To be clear, I am not suggesting you fix the existing bugs in this PR (although that would be very much appreciated). Just that we should decide whether such typedefs ought to be rejected - in which case your new code is fine as-is - or accepted - in which case we need the extra calls to ConvertibleTo and Convert. @mattn What are your thoughts on this point?

return callbackArgTime, nil
default:
return nil, fmt.Errorf("don't know how to convert to %s", typ)
}
}
}

func callbackArgTime(v *C.sqlite3_value) (reflect.Value, error) {
var t time.Time
var err error
var c *C.char
switch C.sqlite3_value_type(v) {
case C.SQLITE_BLOB:
c = (*C.char)(C.sqlite3_value_blob(v))
case C.SQLITE_TEXT:
c = (*C.char)(unsafe.Pointer(C.sqlite3_value_text(v)))
default:
return reflect.Value{}, errors.New("argument must be BLOB or TEXT")
}
lc := C.sqlite3_value_bytes(v)
tv := C.GoStringN(c, lc)
for _, format := range SQLiteTimestampFormats {
if t, err = time.ParseInLocation(format, tv, time.UTC); err == nil {
break
}
}
if err != nil {
return reflect.Value{}, errors.New("argument could not be parsed into known Timestamp formats")
}
return reflect.ValueOf(t), nil
}

func callbackConvertArgs(argv []*C.sqlite3_value, converters []callbackArgConverter, variadic callbackArgConverter) ([]reflect.Value, error) {
var args []reflect.Value

Expand Down Expand Up @@ -335,6 +368,12 @@ func callbackRetNil(ctx *C.sqlite3_context, v reflect.Value) error {
return nil
}

func callbackRetTime(ctx *C.sqlite3_context, v reflect.Value) error {
rv := v.Interface().(time.Time)
C._sqlite3_result_text(ctx, C.CString(rv.Format(SQLiteTimestampFormats[0])))
return nil
}

func callbackRet(typ reflect.Type) (callbackRetConverter, error) {
switch typ.Kind() {
case reflect.Interface:
Expand All @@ -355,7 +394,12 @@ func callbackRet(typ reflect.Type) (callbackRetConverter, error) {
case reflect.Float32, reflect.Float64:
return callbackRetFloat, nil
default:
return nil, fmt.Errorf("don't know how to convert to %s", typ)
switch typ {
case timetype:
return callbackRetTime, nil
default:
return nil, fmt.Errorf("don't know how to convert to %s", typ)
}
}
}

Expand Down
2 changes: 2 additions & 0 deletions callback_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"math"
"reflect"
"testing"
"time"
)

func TestCallbackArgCast(t *testing.T) {
Expand Down Expand Up @@ -66,6 +67,7 @@ func TestCallbackConverters(t *testing.T) {
{uint(0), false},
{float64(0), false},
{float32(0), false},
{time.Now(), false},

{func() {}, true},
{complex64(complex(0, 0)), true},
Expand Down