-
Notifications
You must be signed in to change notification settings - Fork 20
Typed filter functions for queries with boolean/number/string results #43
Conversation
Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech> Typo Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech> Remove unneeded include Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
e0b4c17
to
7115aba
Compare
tools/bindings.go
Outdated
@@ -79,6 +79,15 @@ func ptrToNode(ptr C.uintptr_t) *uast.Node { | |||
return (*uast.Node)(unsafe.Pointer(uintptr(ptr))) | |||
} | |||
|
|||
func initFilter(node *uast.Node, xpath string) (*C.char, int, C.uintptr_t) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it will be great to have a comment that this function acquires findMutex
and caller should unlock it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the second thought, it's better to return some closer func()
that caller should defer.
It can include SetGCPercent
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea.
tools/bindings.go
Outdated
if !C.Filter(ptr, cquery) { | ||
error := C.Error() | ||
return nil, fmt.Errorf("UastFilter() failed: %s", C.GoString(error)) | ||
errorf := fmt.Errorf("UastFilter() failed: %s", C.GoString(error)) | ||
C.free(unsafe.Pointer(error)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these 3 lines can be moved to a separate function, so no caller will be able to leak it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do.
tools/bindings.go
Outdated
func initFilter(node *uast.Node, xpath string) (*C.char, int, C.uintptr_t) { | ||
findMutex.Lock() | ||
cquery := pool.getCstring(xpath) | ||
gcpercent := debug.SetGCPercent(-1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is GC stopped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cargo cult... it was before for the existing functions, I guess the previous programmer found it was needed to avoid problems with C variables, I'll try to remove it and do some intensive testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done some testing with a loop and millions of calls and I didn't find any problem, so I've removed the calls to disable/reenable the GC. We should remember this in case there are odd problems in the future that could be causes by this, but I didn't see any need to disable the GC on cgo documentation.
tools/bindings.h
Outdated
if (!c_ok) { | ||
*ok = 0; | ||
} | ||
*ok = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be in else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ups...
Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
@dennwc changes done, please check |
Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech> Remove unneded import Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
8360e9f
to
9ba708f
Compare
FilterBool
,FilterNumber
,FilterString
.C.free()
s after returns o_O and thus memory leaks.