search

mapbox / node-cpp-skel

Skeleton for bindings to C++ libraries for Node.js using node-addon-api
Creative Commons Zero v1.0 Universal
72 stars 10 forks source link

Clang tidy fixes #66

Closed springmeyer closed 7 years ago

springmeyer commented 7 years ago

Applies fixes to errors that clang-tidy identified - refs #64 / #65

/cc @GretaCB

springmeyer commented 7 years ago

Currently the string data is owned by the class that calls 'AsyncHelloWorker' so that that data will be certain to stay alive. When we move to using node::Buffer then the memory will be owned by the original Buffer passed in from JS.

On Aug 22, 2017, at 2:51 AM, Daniel J. H. notifications@github.com wrote:

@daniel-j-h commented on this pull request.

In src/object_async/hello_async.cpp:

  • AsyncHelloWorker(bool louder, std::string const& name,
  • AsyncHelloWorker(bool louder, const std::string* name, Question re. string_view / string_ref: these abstractions don't own the memory they're pointing to but are basically typed const&s - who owns their memory? The buffer? Then who owns the buffer? Do you expect the buffer to be moved into the worker?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

springmeyer commented 7 years ago

This it green, so I'm going to merge. Per https://github.com/mapbox/node-cpp-skel/pull/66#discussion_r135169735, #67 will be a future followup that will enhance some of the code that was changed here. Having clang-tidy in place now will help us catch errors sooner in that future work.