Sitelet https://github.com/node-ffi/node-ffi/pull/306/files
Skip to content

Allow to pass DynamicLibrary to Library func #306

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.

Sign up for GitHub

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

Open
kolya-ay wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Allow to pass DynamicLibrary to Library func #306

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
17 changes: 9 additions & 8 deletions lib/library.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,18 +31,20 @@ var EXT = Library.EXT = {
* ForeignFunction.
*/

function Library (libfile, funcs, lib) {
debug('creating Library object for', libfile)
function Library (dl, funcs, lib) {
debug('creating Library object for', dl)

if (libfile && libfile.indexOf(EXT) === -1) {
debug('appending library extension to library name', EXT)
libfile += EXT
if (!(dl instanceof DynamicLibrary)) {

@TooTallNate TooTallNate Jun 22, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's do if ('string' === typeof dl || null === dl) instead. I'm paranoid about instanceof and try to avoid it when possible.

@RSully RSully Jun 22, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

any particular reason? instanceof is logically correct, no?

@TooTallNate TooTallNate Jun 22, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

IMO instanceof is an anti-pattern in dynamically typed languages like JavaScript. Consider a DynamicLibrary instance from another "env" (i.e. another copy of node-ffi in the node_modules structure), or another module that implements the DynamicLibrary interface but using a different backend (perhaps libuv's dl functions). Basically the "if it looks like a duck, and quacks like a duck" benefits of dynamic langs are lost with instanceof.

if (dl && dl.indexOf(EXT) === -1) {
debug('appending library extension to library name', EXT)
dl += EXT
}
dl = new DynamicLibrary(dl || null, RTLD_NOW)
}

if (!lib) {
lib = {}
}
var dl = new DynamicLibrary(libfile || null, RTLD_NOW)

Object.keys(funcs || {}).forEach(function (func) {
debug('defining function', func)
Expand All @@ -51,8 +53,7 @@ function Library (libfile, funcs, lib) {
, info = funcs[func]

@TooTallNate TooTallNate Jun 22, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you tack this.path = path in the DynamicLibrary constructor then we can still know the name of the dylib and print it out here (dl.path).

if (fptr.isNull()) {
throw new Error('Library: "' + libfile
+ '" returned NULL function pointer for "' + func + '"')
throw new Error('Library returned NULL function pointer for "' + func + '"')
}

var resultType = info[0]
Expand Down