From 2aff3c0dbcd04c232a7ceb522760fa232157c285 Mon Sep 17 00:00:00 2001 From: John Haley Date: Thu, 19 Feb 2015 18:30:11 -0700 Subject: [PATCH 01/11] Fix callbacks with just return value --- generate/input/callbacks.json | 2 +- generate/templates/filters/returns_info.js | 2 +- .../templates/partials/callback_helpers.cc | 22 +++++++++++++++++-- lib/index.js | 6 ++--- package.json | 2 +- 5 files changed, 26 insertions(+), 8 deletions(-) diff --git a/generate/input/callbacks.json b/generate/input/callbacks.json index 75c4b26e5..0b448c5a8 100644 --- a/generate/input/callbacks.json +++ b/generate/input/callbacks.json @@ -455,7 +455,7 @@ ], "return": { "type": "int", - "noResults": 1, + "noResults": 0, "success": 0, "error": -1 } diff --git a/generate/templates/filters/returns_info.js b/generate/templates/filters/returns_info.js index c1a315aee..ff36e0c51 100644 --- a/generate/templates/filters/returns_info.js +++ b/generate/templates/filters/returns_info.js @@ -14,12 +14,12 @@ module.exports = function(fn, argReturnsOnly, isAsync) { return_info.parsedClassName = (return_info.cppClassName || '').toLowerCase() + "_t"; return_info.returnNameOrName = return_info.returnName || return_info.name; return_info.jsOrCppClassName = return_info.jsClassName || return_info.cppClassName; + return_info.isOutParam = true; result.push(return_info); }); if (!result.length - && !fn.isCallbackFunction && !argReturnsOnly && fn.return && !fn.return.isErrorCode diff --git a/generate/templates/partials/callback_helpers.cc b/generate/templates/partials/callback_helpers.cc index 8cbfc50c0..150e0c899 100644 --- a/generate/templates/partials/callback_helpers.cc +++ b/generate/templates/partials/callback_helpers.cc @@ -86,16 +86,25 @@ void {{ cppClassName }}::{{ cppFunctionName }}_{{ cbFunction.name }}_asyncAfter( {{ cbFunction.return.type }} resultStatus; - {% each cbFunction|returnsInfo true false as _return %} + {% each cbFunction|returnsInfo false true as _return %} if (result.IsEmpty() || result->IsNativeError()) { baton->result = {{ cbFunction.return.error }}; } else if (!result->IsNull() && !result->IsUndefined()) { + {% if _return.isOutParam %} {{ _return.cppClassName }}* wrapper = ObjectWrap::Unwrap<{{ _return.cppClassName }}>(result->ToObject()); wrapper->selfFreeing = false; baton->{{ _return.name }} = wrapper->GetRefValue(); baton->result = {{ cbFunction.return.success }}; + {% else %} + if (result->IsNumber()) { + baton->result = (int) result->ToNumber()->Value(); + } + else { + baton->result = {{ cbFunction.return.noResults }}; + } + {% endif %} } else { baton->result = {{ cbFunction.return.noResults }}; @@ -126,16 +135,25 @@ void {{ cppClassName }}::{{ cppFunctionName }}_{{ cbFunction.name }}_asyncPromis Handle result = resultFn->Call(0, argv); {{ cbFunction.return.type }} resultStatus; - {% each cbFunction|returnsInfo true false as _return %} + {% each cbFunction|returnsInfo false true as _return %} if (result.IsEmpty() || result->IsNativeError()) { baton->result = {{ cbFunction.return.error }}; } else if (!result->IsNull() && !result->IsUndefined()) { + {% if _return.isOutParam %} {{ _return.cppClassName }}* wrapper = ObjectWrap::Unwrap<{{ _return.cppClassName }}>(result->ToObject()); wrapper->selfFreeing = false; baton->{{ _return.name }} = wrapper->GetRefValue(); baton->result = {{ cbFunction.return.success }}; + {% else %} + if (result->IsNumber()) { + baton->result = (int) result->ToNumber()->Value(); + } + else { + baton->result = {{ cbFunction.return.noResults }}; + } + {% endif %} } else { baton->result = {{ cbFunction.return.noResults }}; diff --git a/lib/index.js b/lib/index.js index 686f36778..e4b9431d8 100644 --- a/lib/index.js +++ b/lib/index.js @@ -19,17 +19,17 @@ Index.prototype.entries = function() { var addAll = Index.prototype.addAll; Index.prototype.addAll = function(pathspec, flags, matchedCallback) { - return addAll.call(this, pathspec, flags, matchedCallback, null); + return addAll.call(this, pathspec || "*", flags, matchedCallback, null); }; var removeAll = Index.prototype.removeAll; Index.prototype.removeAll = function(pathspec, matchedCallback) { - return removeAll.call(this, pathspec, matchedCallback, null); + return removeAll.call(this, pathspec || "*", matchedCallback, null); }; var updateAll = Index.prototype.updateAll; Index.prototype.updateAll = function(pathspec, matchedCallback) { - return updateAll.call(this, pathspec, matchedCallback, null); + return updateAll.call(this, pathspec || "*", matchedCallback, null); }; module.exports = Index; diff --git a/package.json b/package.json index 1757a8731..eb5ea2d6a 100644 --- a/package.json +++ b/package.json @@ -62,7 +62,7 @@ "which-native-nodish": "^1.0.3" }, "devDependencies": { - "combyne": "^0.6.4", + "combyne": "^0.6.5", "istanbul": "^0.3.5", "js-beautify": "^1.5.4", "jshint": "^2.6.0", From deeb885db85d13ee0249fea32f9916435a3f420e Mon Sep 17 00:00:00 2001 From: John Haley Date: Thu, 19 Feb 2015 21:31:48 -0700 Subject: [PATCH 02/11] Normalize returns for callbacks --- generate/input/callbacks.json | 2 +- .../templates/partials/callback_helpers.cc | 11 ++++--- .../templates/partials/field_accessors.cc | 29 +++++++++++++++---- test/tests/clone.js | 2 +- 4 files changed, 30 insertions(+), 14 deletions(-) diff --git a/generate/input/callbacks.json b/generate/input/callbacks.json index 0b448c5a8..46dbb4654 100644 --- a/generate/input/callbacks.json +++ b/generate/input/callbacks.json @@ -495,7 +495,7 @@ ], "return": { "type": "int", - "noResults": 1, + "noResults": 0, "success": 0, "error": -1 } diff --git a/generate/templates/partials/callback_helpers.cc b/generate/templates/partials/callback_helpers.cc index 150e0c899..a4aad8713 100644 --- a/generate/templates/partials/callback_helpers.cc +++ b/generate/templates/partials/callback_helpers.cc @@ -22,8 +22,10 @@ this_thread::sleep_for(chrono::milliseconds(1)); } - {% each cbFunction|returnsInfo true false as _return %} + {% each cbFunction|returnsInfo false true as _return %} + {% if _return.isOutParam %} *{{ _return.name }} = *baton->{{ _return.name }}; + {% endif %} {% endeach %} return baton->result; @@ -84,8 +86,6 @@ void {{ cppClassName }}::{{ cppFunctionName }}_{{ cbFunction.name }}_asyncAfter( } } - {{ cbFunction.return.type }} resultStatus; - {% each cbFunction|returnsInfo false true as _return %} if (result.IsEmpty() || result->IsNativeError()) { baton->result = {{ cbFunction.return.error }}; @@ -99,7 +99,7 @@ void {{ cppClassName }}::{{ cppFunctionName }}_{{ cbFunction.name }}_asyncAfter( baton->result = {{ cbFunction.return.success }}; {% else %} if (result->IsNumber()) { - baton->result = (int) result->ToNumber()->Value(); + baton->result = (int)result->ToNumber()->Value(); } else { baton->result = {{ cbFunction.return.noResults }}; @@ -133,7 +133,6 @@ void {{ cppClassName }}::{{ cppFunctionName }}_{{ cbFunction.name }}_asyncPromis if (isFulfilled->Value()) { NanCallback* resultFn = new NanCallback(promise->Get(NanNew("value")).As()); Handle result = resultFn->Call(0, argv); - {{ cbFunction.return.type }} resultStatus; {% each cbFunction|returnsInfo false true as _return %} if (result.IsEmpty() || result->IsNativeError()) { @@ -148,7 +147,7 @@ void {{ cppClassName }}::{{ cppFunctionName }}_{{ cbFunction.name }}_asyncPromis baton->result = {{ cbFunction.return.success }}; {% else %} if (result->IsNumber()) { - baton->result = (int) result->ToNumber()->Value(); + baton->result = (int)result->ToNumber()->Value(); } else { baton->result = {{ cbFunction.return.noResults }}; diff --git a/generate/templates/partials/field_accessors.cc b/generate/templates/partials/field_accessors.cc index cd3a1e4a0..92c9ba60f 100644 --- a/generate/templates/partials/field_accessors.cc +++ b/generate/templates/partials/field_accessors.cc @@ -94,8 +94,10 @@ this_thread::sleep_for(chrono::milliseconds(1)); } - {% each field|returnsInfo true false as _return %} + {% each field|returnsInfo false true as _return %} + {% if _return.isOutParam %} *{{ _return.name }} = *baton->{{ _return.name }}; + {% endif %} {% endeach %} return baton->result; @@ -173,18 +175,25 @@ } } - {{ field.return.type }} resultStatus; - - {% each field|returnsInfo true false as _return %} + {% each field|returnsInfo false true as _return %} if (result.IsEmpty() || result->IsNativeError()) { baton->result = {{ field.return.error }}; } else if (!result->IsNull() && !result->IsUndefined()) { + {% if _return.isOutParam %} {{ _return.cppClassName }}* wrapper = ObjectWrap::Unwrap<{{ _return.cppClassName }}>(result->ToObject()); wrapper->selfFreeing = false; baton->{{ _return.name }} = wrapper->GetRefValue(); baton->result = {{ field.return.success }}; + {% else %} + if (result->IsNumber()) { + baton->result = (int)result->ToNumber()->Value(); + } + else { + baton->result = {{ field.return.noResults }}; + } + {% endif %} } else { baton->result = {{ field.return.noResults }}; @@ -213,18 +222,26 @@ if (isFulfilled->Value()) { NanCallback* resultFn = new NanCallback(promise->Get(NanNew("value")).As()); Handle result = resultFn->Call(0, argv); - {{ field.return.type }} resultStatus; - {% each field|returnsInfo true false as _return %} + {% each field|returnsInfo false true as _return %} if (result.IsEmpty() || result->IsNativeError()) { baton->result = {{ field.return.error }}; } else if (!result->IsNull() && !result->IsUndefined()) { + {% if _return.isOutParam %} {{ _return.cppClassName }}* wrapper = ObjectWrap::Unwrap<{{ _return.cppClassName }}>(result->ToObject()); wrapper->selfFreeing = false; baton->{{ _return.name }} = wrapper->GetRefValue(); baton->result = {{ field.return.success }}; + {% else %} + if (result->IsNumber()) { + baton->result = (int)result->ToNumber()->Value(); + } + else{ + baton->result = {{ field.return.noResults }}; + } + {% endif %} } else { baton->result = {{ field.return.noResults }}; diff --git a/test/tests/clone.js b/test/tests/clone.js index bbe6dfa4e..6f485672e 100644 --- a/test/tests/clone.js +++ b/test/tests/clone.js @@ -19,7 +19,7 @@ describe("Clone", function() { var sshPrivateKey = local("../id_rsa"); // Set a reasonable timeout here now that our repository has grown. - this.timeout(15000); + this.timeout(30000); beforeEach(function() { return NodeGit.Promise.all([ From 0a7ac52d10583f245bfd0979a2a43fe1799a988f Mon Sep 17 00:00:00 2001 From: John Haley Date: Thu, 19 Feb 2015 21:43:42 -0700 Subject: [PATCH 03/11] Force functions with callbacks to be async If a function has a callback then it must be async. If it isn't then we'll hit a deadlocked state when waiting for the JS callback. --- generate/scripts/helpers.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/generate/scripts/helpers.js b/generate/scripts/helpers.js index da7be6cf1..cdc288898 100644 --- a/generate/scripts/helpers.js +++ b/generate/scripts/helpers.js @@ -302,6 +302,11 @@ var Helpers = { var argOverrides = fnOverrides.args || {}; fnDef.args.forEach(function(arg) { Helpers.decorateArg(arg, fnDef.args, typeDef, fnDef, argOverrides[arg.name] || {}, enums); + + // if a function has any callbacks then it MUST be async + if (arg.isCallbackFunction) { + fnDef.isAsync = true; + } }); if (fnDef.return) { From 530290dcc1b498f5dbb5f14e298619cad1bcace1 Mon Sep 17 00:00:00 2001 From: John Haley Date: Fri, 20 Feb 2015 05:36:31 -0700 Subject: [PATCH 04/11] Handle global payloads in functions --- generate/scripts/helpers.js | 1 + generate/templates/partials/async_function.cc | 32 ++++++++++++++++++- .../templates/partials/callback_helpers.cc | 4 +++ generate/templates/partials/sync_function.cc | 24 ++------------ generate/templates/templates/class_header.h | 22 +++++++++---- 5 files changed, 53 insertions(+), 30 deletions(-) diff --git a/generate/scripts/helpers.js b/generate/scripts/helpers.js index cdc288898..49cf3be44 100644 --- a/generate/scripts/helpers.js +++ b/generate/scripts/helpers.js @@ -121,6 +121,7 @@ var Helpers = { processPayload: function(field, allFields) { if (field.name === "payload") { field.payloadFor = "*"; + field.globalPayload = true; field.isOptional = true; } else { diff --git a/generate/templates/partials/async_function.cc b/generate/templates/partials/async_function.cc index e65d8ff08..0e989a238 100644 --- a/generate/templates/partials/async_function.cc +++ b/generate/templates/partials/async_function.cc @@ -12,6 +12,12 @@ NAN_METHOD({{ cppClassName }}::{{ cppFunctionName }}) { baton->error_code = GIT_OK; baton->error = NULL; + {%each args|argsInfo as arg %} + {%if arg.globalPayload %} + {{ cppFunctionName }}_globalPayload* globalPayload = new {{ cppFunctionName }}_globalPayload; + {%endif%} + {%endeach%} + {%each args|argsInfo as arg %} {%if not arg.isReturn %} {%if arg.isSelf %} @@ -23,10 +29,16 @@ NAN_METHOD({{ cppClassName }}::{{ cppFunctionName }}) { } else { baton->{{ arg.name}} = {{ cppFunctionName }}_{{ arg.name }}_cppCallback; + {%if arg.payload.globalPayload %} + globalPayload->{{ arg.name }} = new NanCallback(args[{{ arg.jsArg }}].As()); + {%else%} baton->{{ arg.payload.name }} = new NanCallback(args[{{ arg.jsArg }}].As()); + {%endif%} } {%elsif arg.payloadFor %} - {%-- payloads are ignored --%} + {%if arg.globalPayload %} + baton->{{ arg.name }} = globalPayload; + {%endif%} {%elsif arg.name %} {%partial convertFromV8 arg%} {%if not arg.payloadFor %} @@ -134,7 +146,16 @@ void {{ cppClassName }}::{{ cppFunctionName }}Worker::HandleOKCallback() { free((void*)baton->{{ arg.name }}); } {%elsif arg.isCallbackFunction %} + {%if not arg.payload.globalPayload %} delete baton->{{ arg.payload.name }}; + {%endif%} + {%elsif arg.globalPayload %} + {%each args|argsInfo as cbArg %} + {%if cbArg.isCallbackFunction %} + delete (({{ cppFunctionName}}_globalPayload*)baton->{{ arg.name }})->{{ cbArg.name }}; + {%endif%} + {%endeach%} + free((void *)baton->{{ arg.name }}); {%else%} free((void*)baton->{{ arg.name }}); {%endif%} @@ -159,7 +180,16 @@ void {{ cppClassName }}::{{ cppFunctionName }}Worker::HandleOKCallback() { free((void *)baton->{{ arg.name }}); } {%elsif arg.isCallbackFunction %} + {%if not arg.payload.globalPayload %} delete (NanCallback *)baton->{{ arg.payload.name }}; + {%endif%} + {%elsif arg.globalPayload %} + {%each args|argsInfo as cbArg %} + {%if cbArg.isCallbackFunction %} + delete (({{ cppFunctionName}}_globalPayload*)baton->{{ arg.name }})->{{ cbArg.name }}; + {%endif%} + {%endeach%} + free((void *)baton->{{ arg.name }}); {%endif%} {%endeach%} diff --git a/generate/templates/partials/callback_helpers.cc b/generate/templates/partials/callback_helpers.cc index a4aad8713..e8209c484 100644 --- a/generate/templates/partials/callback_helpers.cc +++ b/generate/templates/partials/callback_helpers.cc @@ -44,7 +44,11 @@ void {{ cppClassName }}::{{ cppFunctionName }}_{{ cbFunction.name }}_asyncAfter( {% each cbFunction.args|argsInfo as arg %} {% if arg | isPayload %} + {% if cbFunction.payload.globalPayload %} + NanCallback* callback = (({{ cppFunctionName }}_globalPayload*)baton->{{ arg.name }})->{{ cbFunction.name }}; + {% else %} NanCallback* callback = (NanCallback *)baton->{{ arg.name }}; + {% endif %} {% endif %} {% endeach %} diff --git a/generate/templates/partials/sync_function.cc b/generate/templates/partials/sync_function.cc index eee87f4cc..8a8aab89e 100644 --- a/generate/templates/partials/sync_function.cc +++ b/generate/templates/partials/sync_function.cc @@ -15,27 +15,20 @@ NAN_METHOD({{ cppClassName }}::{{ cppFunctionName }}) { {%each args|argsInfo as arg %} {%if not arg.isSelf %} {%if not arg.isReturn %} - {%if not arg.isCallbackFunction %} - {%if not arg.payloadFor %} - {%partial convertFromV8 arg %} - {%if arg.saveArg %} + {%partial convertFromV8 arg %} + {%if arg.saveArg %} Handle {{ arg.name }}(args[{{ arg.jsArg }}]->ToObject()); {{ cppClassName }} *thisObj = ObjectWrap::Unwrap<{{ cppClassName }}>(args.This()); NanDisposePersistent(thisObj->{{ cppFunctionName }}_{{ arg.name }}); NanAssignPersistent(thisObj->{{ cppFunctionName }}_{{ arg.name }}, {{ arg.name }}); - {%endif%} - {%endif%} {%endif%} {%endif%} {%endif%} {%endeach%} {%each args|argsInfo as arg %} - {%if arg.isCallbackFunction %} -NanCallback* {{ arg.name }}_callback = new NanCallback(args[{{ arg.jsArg }}].As()); - {%endif%} {%endeach%} {%if .|hasReturns %} {{ return.cType }} result = {%endif%}{{ cFunctionName }}( @@ -47,11 +40,6 @@ NanCallback* {{ arg.name }}_callback = new NanCallback(args[{{ arg.jsArg }}].As< ObjectWrap::Unwrap<{{ arg.cppClassName }}>(args.This())->GetValue() {%elsif arg.isReturn %} {{ arg.name }} - {%elsif arg.isCallbackFunction %} -{{ cppFunctionName }}_{{ arg.name }}_cppCallback, -{{ arg.name }}_callback - {%elsif arg.payloadFor %} -{%-- payloads are handled inside of the callback condition --%} {%else%} from_{{ arg.name }} {%endif%} @@ -59,12 +47,6 @@ from_{{ arg.name }} {%endeach%} ); -{%each args|argsInfo as arg %} - {%if arg.isCallbackFunction %} -delete {{ arg.name }}_callback; - {%endif%} -{%endeach%} - {%if return.isErrorCode %} if (result != GIT_OK) { {%each args|argsInfo as arg %} @@ -120,5 +102,3 @@ delete {{ arg.name }}_callback; {%endif%} {%endif%} } - -{%partial callbackHelpers .%} diff --git a/generate/templates/templates/class_header.h b/generate/templates/templates/class_header.h index cb6abfde4..6f64267bf 100644 --- a/generate/templates/templates/class_header.h +++ b/generate/templates/templates/class_header.h @@ -135,17 +135,25 @@ class {{ cppClassName }} : public ObjectWrap { }; {%endif%} - {%each function.args as arg %} - {%if arg.payloadFor %} - - Persistent {{ function.cppFunctionName }}_{{ arg.name }}; - {%endif%} - {%endeach%} - static NAN_METHOD({{ function.cppFunctionName }}); {%endif%} {%endeach%} + {%each functions as function%} + {%each function.args as arg %} + {% if arg.globalPayload %} + + struct {{ function.cppFunctionName }}_globalPayload { + {%each function.args as arg %} + {%if arg.isCallbackFunction %} + NanCallback * {{ arg.name }}; + {%endif%} + {%endeach%} + }; + {%endif%} + {%endeach%} + {%endeach%} + {%if cType%} {{ cType }} *raw; {%endif%} From e954ae57b105dc6849607d7e01980d1b6441c735 Mon Sep 17 00:00:00 2001 From: John Haley Date: Mon, 23 Feb 2015 09:29:50 -0700 Subject: [PATCH 05/11] Fix promises not being returned in index test --- test/tests/index.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/tests/index.js b/test/tests/index.js index babf4cbb7..4507cb0f2 100644 --- a/test/tests/index.js +++ b/test/tests/index.js @@ -43,7 +43,7 @@ describe("Index", function() { var fileNames = Object.keys(fileContent); return Promise.all(fileNames.map(function(fileName) { - fse.writeFile(path.join(repo.workdir(), fileName), fileContent[fileName]); + return fse.writeFile(path.join(repo.workdir(), fileName), fileContent[fileName]); })) .then(function() { return index.addAll(); @@ -57,7 +57,7 @@ describe("Index", function() { }) .then(function() { return Promise.all(fileNames.map(function(fileName) { - fse.remove(path.join(repo.workdir(), fileName)); + return fse.remove(path.join(repo.workdir(), fileName)); })); }) .then(function() { @@ -76,7 +76,7 @@ describe("Index", function() { var fileNames = Object.keys(fileContent); return Promise.all(fileNames.map(function(fileName) { - fse.writeFile(path.join(repo.workdir(), fileName), fileContent[fileName]); + return fse.writeFile(path.join(repo.workdir(), fileName), fileContent[fileName]); })) .then(function() { return index.addAll(); @@ -99,7 +99,7 @@ describe("Index", function() { }) .then(function() { return Promise.all(fileNames.map(function(fileName) { - fse.remove(path.join(repo.workdir(), fileName)); + return fse.remove(path.join(repo.workdir(), fileName)); })); }) .then(function() { @@ -117,7 +117,7 @@ describe("Index", function() { var fileNames = Object.keys(fileContent); return Promise.all(fileNames.map(function(fileName) { - fse.writeFile(path.join(repo.workdir(), fileName), fileContent[fileName]); + return fse.writeFile(path.join(repo.workdir(), fileName), fileContent[fileName]); })) .then(function() { return index.addAll(); From 67c6eb2832d3286d5d21017e04ff443a8937a606 Mon Sep 17 00:00:00 2001 From: John Haley Date: Mon, 23 Feb 2015 10:09:00 -0700 Subject: [PATCH 06/11] Fix git_strarray when an actual array is passed in --- generate/templates/manual/src/str_array_converter.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/generate/templates/manual/src/str_array_converter.cc b/generate/templates/manual/src/str_array_converter.cc index 131989c64..1f85a8bda 100644 --- a/generate/templates/manual/src/str_array_converter.cc +++ b/generate/templates/manual/src/str_array_converter.cc @@ -30,7 +30,7 @@ git_strarray *StrArrayConverter::ConvertArray(Array *val) { git_strarray *result; for(int i = 0; i < count; i++) { - NanUtf8String entry(val->CloneElementAt(i)); + NanUtf8String entry(val->Get(i)); strings[i] = *entry; } From 07443200e2ee200577e1d31cf63f3f975ede5d50 Mon Sep 17 00:00:00 2001 From: John Haley Date: Mon, 23 Feb 2015 10:53:11 -0700 Subject: [PATCH 07/11] Cleanup memory from global payloads --- generate/templates/partials/async_function.cc | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/generate/templates/partials/async_function.cc b/generate/templates/partials/async_function.cc index 0e989a238..9eb0d0d7c 100644 --- a/generate/templates/partials/async_function.cc +++ b/generate/templates/partials/async_function.cc @@ -180,7 +180,13 @@ void {{ cppClassName }}::{{ cppFunctionName }}Worker::HandleOKCallback() { free((void *)baton->{{ arg.name }}); } {%elsif arg.isCallbackFunction %} - {%if not arg.payload.globalPayload %} + {%if arg.payload.globalPayload %} + {%each args|argsInfo as cbArg %} + {%if cbArg.isCallbackFunction %} + delete (({{ cppFunctionName }}_globalPayload*)baton->{{ arg.name }})->{{ cbArg.name }}; + {%endif%} + {%endeach%} + {%else%} delete (NanCallback *)baton->{{ arg.payload.name }}; {%endif%} {%elsif arg.globalPayload %} From d40d7919f6f3427778749c3236cc11ab678146da Mon Sep 17 00:00:00 2001 From: John Haley Date: Mon, 23 Feb 2015 10:56:36 -0700 Subject: [PATCH 08/11] Promisify writeFile by hand in index tests --- test/tests/index.js | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/test/tests/index.js b/test/tests/index.js index 4507cb0f2..f5e9f4cbf 100644 --- a/test/tests/index.js +++ b/test/tests/index.js @@ -5,6 +5,10 @@ var Promise = require("nodegit-promise"); var promisify = require("promisify-node"); var fse = promisify(require("fs-extra")); +var writeFile = promisify(function(filename, data, callback) { + return require("fs").writeFile(filename, data, {}, callback); +}); + describe("Index", function() { var Repository = require(local("../../lib/repository")); @@ -43,7 +47,7 @@ describe("Index", function() { var fileNames = Object.keys(fileContent); return Promise.all(fileNames.map(function(fileName) { - return fse.writeFile(path.join(repo.workdir(), fileName), fileContent[fileName]); + return writeFile(path.join(repo.workdir(), fileName), fileContent[fileName]); })) .then(function() { return index.addAll(); @@ -76,7 +80,7 @@ describe("Index", function() { var fileNames = Object.keys(fileContent); return Promise.all(fileNames.map(function(fileName) { - return fse.writeFile(path.join(repo.workdir(), fileName), fileContent[fileName]); + return writeFile(path.join(repo.workdir(), fileName), fileContent[fileName]); })) .then(function() { return index.addAll(); @@ -117,7 +121,7 @@ describe("Index", function() { var fileNames = Object.keys(fileContent); return Promise.all(fileNames.map(function(fileName) { - return fse.writeFile(path.join(repo.workdir(), fileName), fileContent[fileName]); + return writeFile(path.join(repo.workdir(), fileName), fileContent[fileName]); })) .then(function() { return index.addAll(); From 99f9fdaa7904fc6159e08db9b65779698db59d36 Mon Sep 17 00:00:00 2001 From: John Haley Date: Mon, 23 Feb 2015 11:04:27 -0700 Subject: [PATCH 09/11] Fix linter errors [ci skip] --- test/tests/index.js | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/test/tests/index.js b/test/tests/index.js index f5e9f4cbf..4521a2d72 100644 --- a/test/tests/index.js +++ b/test/tests/index.js @@ -47,7 +47,9 @@ describe("Index", function() { var fileNames = Object.keys(fileContent); return Promise.all(fileNames.map(function(fileName) { - return writeFile(path.join(repo.workdir(), fileName), fileContent[fileName]); + return writeFile( + path.join(repo.workdir(), fileName), + fileContent[fileName]); })) .then(function() { return index.addAll(); @@ -80,7 +82,9 @@ describe("Index", function() { var fileNames = Object.keys(fileContent); return Promise.all(fileNames.map(function(fileName) { - return writeFile(path.join(repo.workdir(), fileName), fileContent[fileName]); + return writeFile( + path.join(repo.workdir(), fileName), + fileContent[fileName]); })) .then(function() { return index.addAll(); @@ -121,7 +125,9 @@ describe("Index", function() { var fileNames = Object.keys(fileContent); return Promise.all(fileNames.map(function(fileName) { - return writeFile(path.join(repo.workdir(), fileName), fileContent[fileName]); + return writeFile( + path.join(repo.workdir(), fileName), + fileContent[fileName]); })) .then(function() { return index.addAll(); From 3b9b63725843da58932b4d65b11a292848bf12fd Mon Sep 17 00:00:00 2001 From: John Haley Date: Mon, 23 Feb 2015 15:29:57 -0700 Subject: [PATCH 10/11] Clean up creation/deletion of global payloads --- generate/templates/partials/async_function.cc | 28 ++++++------------- generate/templates/templates/class_header.h | 20 ++++++++++++- 2 files changed, 27 insertions(+), 21 deletions(-) diff --git a/generate/templates/partials/async_function.cc b/generate/templates/partials/async_function.cc index 9eb0d0d7c..2ed5089c3 100644 --- a/generate/templates/partials/async_function.cc +++ b/generate/templates/partials/async_function.cc @@ -25,7 +25,11 @@ NAN_METHOD({{ cppClassName }}::{{ cppFunctionName }}) { {%elsif arg.isCallbackFunction %} if (!args[{{ arg.jsArg }}]->IsFunction()) { baton->{{ arg.name }} = NULL; + {%if arg.payload.globalPayload %} + globalPayload->{{ arg.name }} = NULL; + {%else%} baton->{{ arg.payload.name }} = NULL; + {%endif%} } else { baton->{{ arg.name}} = {{ cppFunctionName }}_{{ arg.name }}_cppCallback; @@ -150,12 +154,7 @@ void {{ cppClassName }}::{{ cppFunctionName }}Worker::HandleOKCallback() { delete baton->{{ arg.payload.name }}; {%endif%} {%elsif arg.globalPayload %} - {%each args|argsInfo as cbArg %} - {%if cbArg.isCallbackFunction %} - delete (({{ cppFunctionName}}_globalPayload*)baton->{{ arg.name }})->{{ cbArg.name }}; - {%endif%} - {%endeach%} - free((void *)baton->{{ arg.name }}); + delete ({{ cppFunctionName}}_globalPayload*)baton->{{ arg.name }}; {%else%} free((void*)baton->{{ arg.name }}); {%endif%} @@ -180,22 +179,11 @@ void {{ cppClassName }}::{{ cppFunctionName }}Worker::HandleOKCallback() { free((void *)baton->{{ arg.name }}); } {%elsif arg.isCallbackFunction %} - {%if arg.payload.globalPayload %} - {%each args|argsInfo as cbArg %} - {%if cbArg.isCallbackFunction %} - delete (({{ cppFunctionName }}_globalPayload*)baton->{{ arg.name }})->{{ cbArg.name }}; - {%endif%} - {%endeach%} - {%else%} - delete (NanCallback *)baton->{{ arg.payload.name }}; + {%if not arg.payload.globalPayload %} + delete baton->{{ arg.payload.name }}; {%endif%} {%elsif arg.globalPayload %} - {%each args|argsInfo as cbArg %} - {%if cbArg.isCallbackFunction %} - delete (({{ cppFunctionName}}_globalPayload*)baton->{{ arg.name }})->{{ cbArg.name }}; - {%endif%} - {%endeach%} - free((void *)baton->{{ arg.name }}); + delete ({{ cppFunctionName}}_globalPayload*)baton->{{ arg.name }}; {%endif%} {%endeach%} diff --git a/generate/templates/templates/class_header.h b/generate/templates/templates/class_header.h index 6f64267bf..b633ce200 100644 --- a/generate/templates/templates/class_header.h +++ b/generate/templates/templates/class_header.h @@ -146,9 +146,27 @@ class {{ cppClassName }} : public ObjectWrap { struct {{ function.cppFunctionName }}_globalPayload { {%each function.args as arg %} {%if arg.isCallbackFunction %} - NanCallback * {{ arg.name }}; + NanCallback * {{ arg.name }}; {%endif%} {%endeach%} + + {{ function.cppFunctionName }}_globalPayload() { + {%each function.args as arg %} + {%if arg.isCallbackFunction %} + {{ arg.name }} = NULL; + {%endif%} + {%endeach%} + } + + ~{{ function.cppFunctionName }}_globalPayload() { + {%each function.args as arg %} + {%if arg.isCallbackFunction %} + if ({{ arg.name }} != NULL) { + delete {{ arg.name }}; + } + {%endif%} + {%endeach%} + } }; {%endif%} {%endeach%} From dece787539ac204b3756017c9f50ccfeac2890d2 Mon Sep 17 00:00:00 2001 From: John Haley Date: Mon, 23 Feb 2015 16:18:15 -0700 Subject: [PATCH 11/11] Clean up callbacks in structs --- generate/templates/partials/field_accessors.cc | 4 ++++ generate/templates/templates/struct_content.cc | 17 ++++++++++++++--- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/generate/templates/partials/field_accessors.cc b/generate/templates/partials/field_accessors.cc index 92c9ba60f..dcd83937e 100644 --- a/generate/templates/partials/field_accessors.cc +++ b/generate/templates/partials/field_accessors.cc @@ -42,6 +42,10 @@ wrapper->raw->{{ field.name }} = {% if not field.cType | isPointer %}*{% endif %}ObjectWrap::Unwrap<{{ field.cppClassName }}>(value->ToObject())->GetValue(); {% elsif field.isCallbackFunction %} + if (wrapper->{{ field.name }} != NULL) { + delete wrapper->{{ field.name }}; + } + if (value->IsFunction()) { if (!wrapper->raw->{{ field.name }}) { wrapper->raw->{{ field.name }} = ({{ field.cType }}){{ field.name }}_cppCallback; diff --git a/generate/templates/templates/struct_content.cc b/generate/templates/templates/struct_content.cc index 2f67fbdde..40a343ae5 100644 --- a/generate/templates/templates/struct_content.cc +++ b/generate/templates/templates/struct_content.cc @@ -41,8 +41,19 @@ using namespace std; } {{ cppClassName }}::~{{ cppClassName }}() { - // This is going to cause memory leaks. We'll have to solve that later - // TODO: Clean up memory better + {% each fields|fieldsInfo as field %} + {% if not field.ignore %} + {% if not field.isEnum %} + {% if field.isCallbackFunction %} + if (this->{{ field.name }} != NULL) { + delete this->{{ field.name }}; + this->raw->{{ fields|payloadFor field.name }} = NULL; + } + {% endif %} + {% endif %} + {% endif %} + {% endeach %} + if (this->selfFreeing) { free(this->raw); } @@ -65,7 +76,7 @@ void {{ cppClassName }}::ConstructFields() { // the current instance this->raw->{{ field.name }} = NULL; this->raw->{{ fields|payloadFor field.name }} = (void *)this; - this->{{ field.name }} = new NanCallback(); + this->{{ field.name }} = NULL; {% elsif field.payloadFor %} Local {{ field.name }} = NanUndefined();