From 1be9c3795e928ca193f047c2023ce910fe2f61e5 Mon Sep 17 00:00:00 2001 From: Tyler Wanek Date: Fri, 12 Oct 2018 16:37:16 -0700 Subject: [PATCH 01/13] Clean up spacing in templates --- generate/templates/filters/returns_info.js | 24 +++ generate/templates/partials/async_function.cc | 167 +++++++++-------- generate/templates/partials/sync_function.cc | 175 +++++++++--------- 3 files changed, 200 insertions(+), 166 deletions(-) diff --git a/generate/templates/filters/returns_info.js b/generate/templates/filters/returns_info.js index 7eaf26e7a..eeefa23c0 100644 --- a/generate/templates/filters/returns_info.js +++ b/generate/templates/filters/returns_info.js @@ -4,6 +4,18 @@ module.exports = function(fn, argReturnsOnly, isAsync) { var result = []; var args = fn.args || []; + // We will use this to figure out the index + // of arguments, because sync functions + // need to respect ownership to an arbitrary + // parameter that is labeled by name in the + // descriptor, and we won't have access to + // any sort of string to argument index + // in the template. + var nameToArgIndex = {}; + args.forEach(function (arg, index) { + nameToArgIndex[arg.name] = index; + }); + args.forEach(function (arg) { if (!arg.isReturn) return; @@ -11,6 +23,7 @@ module.exports = function(fn, argReturnsOnly, isAsync) { return_info.__proto__ = arg; + return_info.isAsync = isAsync; return_info.parsedName = isAsync ? "baton->" + return_info.name : return_info.name; return_info.isCppClassIntType = ~['Uint32', 'Int32'].indexOf(return_info.cppClassName); return_info.needsDereference @@ -21,6 +34,15 @@ module.exports = function(fn, argReturnsOnly, isAsync) { return_info.returnNameOrName = return_info.returnName || return_info.name; return_info.jsOrCppClassName = return_info.jsClassName || return_info.cppClassName; return_info.isOutParam = true; + return_info.hasOwner = return_info.ownedBy || return_info.ownedByThis; + return_info.ownedByIndex = -1; + + // Here we convert ownedBy, which is the name of the parameter + // that owns this result to the argument index. + // sync functions will need to know this. + if (!isAsync && return_info.ownedBy) { + return_info.ownedByIndex = nameToArgIndex[return_info.ownedBy]; + } result.push(return_info); }); @@ -33,6 +55,8 @@ module.exports = function(fn, argReturnsOnly, isAsync) { var return_info = {}; return_info.__proto__ = fn.return; + + return_info.isAsync = isAsync; return_info.parsedName = return_info.name && isAsync ? "baton->" + return_info.name : "result"; return_info.isCppClassIntType = ~['Uint32', 'Int32'].indexOf(return_info.cppClassName); return_info.parsedClassName = (return_info.cppClassName || '').toLowerCase() + "_t"; diff --git a/generate/templates/partials/async_function.cc b/generate/templates/partials/async_function.cc index d82898292..c4cdb115d 100644 --- a/generate/templates/partials/async_function.cc +++ b/generate/templates/partials/async_function.cc @@ -13,41 +13,41 @@ NAN_METHOD({{ cppClassName }}::{{ cppFunctionName }}) { {%each args|argsInfo as arg %} {%if arg.globalPayload %} - {{ cppFunctionName }}_globalPayload* globalPayload = new {{ cppFunctionName }}_globalPayload; + {{ cppFunctionName }}_globalPayload* globalPayload = new {{ cppFunctionName }}_globalPayload; {%endif%} {%endeach%} {%each args|argsInfo as arg %} {%if not arg.isReturn %} {%if arg.isSelf %} - baton->{{ arg.name }} = Nan::ObjectWrap::Unwrap<{{ arg.cppClassName }}>(info.This())->GetValue(); + baton->{{ arg.name }} = Nan::ObjectWrap::Unwrap<{{ arg.cppClassName }}>(info.This())->GetValue(); {%elsif arg.isCallbackFunction %} - if (!info[{{ arg.jsArg }}]->IsFunction()) { - baton->{{ arg.name }} = NULL; + if (!info[{{ arg.jsArg }}]->IsFunction()) { + baton->{{ arg.name }} = NULL; {%if arg.payload.globalPayload %} - globalPayload->{{ arg.name }} = NULL; + globalPayload->{{ arg.name }} = NULL; {%else%} - baton->{{ arg.payload.name }} = NULL; + baton->{{ arg.payload.name }} = NULL; {%endif%} - } - else { - baton->{{ arg.name}} = {{ cppFunctionName }}_{{ arg.name }}_cppCallback; - {%if arg.payload.globalPayload %} - globalPayload->{{ arg.name }} = new Nan::Callback(info[{{ arg.jsArg }}].As()); - {%else%} - baton->{{ arg.payload.name }} = new Nan::Callback(info[{{ arg.jsArg }}].As()); - {%endif%} - } + } + else { + baton->{{ arg.name}} = {{ cppFunctionName }}_{{ arg.name }}_cppCallback; + {%if arg.payload.globalPayload %} + globalPayload->{{ arg.name }} = new Nan::Callback(info[{{ arg.jsArg }}].As()); + {%else%} + baton->{{ arg.payload.name }} = new Nan::Callback(info[{{ arg.jsArg }}].As()); + {%endif%} + } {%elsif arg.payloadFor %} {%if arg.globalPayload %} - baton->{{ arg.name }} = globalPayload; + baton->{{ arg.name }} = globalPayload; {%endif%} {%elsif arg.name %} - {%partial convertFromV8 arg%} + {%partial convertFromV8 arg%} {%if not arg.payloadFor %} - baton->{{ arg.name }} = from_{{ arg.name }}; + baton->{{ arg.name }} = from_{{ arg.name }}; {%if arg | isOid %} - baton->{{ arg.name }}NeedsFree = info[{{ arg.jsArg }}]->IsString(); + baton->{{ arg.name }}NeedsFree = info[{{ arg.jsArg }}]->IsString(); {%endif%} {%endif%} {%endif%} @@ -62,13 +62,14 @@ NAN_METHOD({{ cppClassName }}::{{ cppFunctionName }}) { Nan::Callback *callback = new Nan::Callback(v8::Local::Cast(info[{{args|jsArgsCount}}])); {{ cppFunctionName }}Worker *worker = new {{ cppFunctionName }}Worker(baton, callback); + {%each args|argsInfo as arg %} {%if not arg.isReturn %} {%if arg.isSelf %} - worker->SaveToPersistent("{{ arg.name }}", info.This()); + worker->SaveToPersistent("{{ arg.name }}", info.This()); {%elsif not arg.isCallbackFunction %} - if (!info[{{ arg.jsArg }}]->IsUndefined() && !info[{{ arg.jsArg }}]->IsNull()) - worker->SaveToPersistent("{{ arg.name }}", info[{{ arg.jsArg }}]->ToObject()); + if (!info[{{ arg.jsArg }}]->IsUndefined() && !info[{{ arg.jsArg }}]->IsNull()) + worker->SaveToPersistent("{{ arg.name }}", info[{{ arg.jsArg }}]->ToObject()); {%endif%} {%endif%} {%endeach%} @@ -81,41 +82,45 @@ void {{ cppClassName }}::{{ cppFunctionName }}Worker::Execute() { giterr_clear(); { - LockMaster lockMaster(/*asyncAction: */true{%each args|argsInfo as arg %} - {%if arg.cType|isPointer%}{%if not arg.cType|isDoublePointer%} - ,baton->{{ arg.name }} - {%endif%}{%endif%} - {%endeach%}); + LockMaster lockMaster( + /*asyncAction: */true + {%each args|argsInfo as arg %} + {%if arg.cType|isPointer%} + {%if not arg.cType|isDoublePointer%} + ,baton->{{ arg.name }} + {%endif%} + {%endif%} + {%endeach%} + ); {%if .|hasReturnType %} - {{ return.cType }} result = {{ cFunctionName }}( + {{ return.cType }} result = {{ cFunctionName }}( {%else%} - {{ cFunctionName }}( + {{ cFunctionName }}( {%endif%} {%-- Insert Function Arguments --%} {%each args|argsInfo as arg %} {%-- turn the pointer into a ref --%} - {%if arg.isReturn|and arg.cType|isDoublePointer %}&{%endif%}baton->{{ arg.name }}{%if not arg.lastArg %},{%endif%} - + {%if arg.isReturn|and arg.cType|isDoublePointer %}&{%endif%}baton->{{ arg.name }}{%if not arg.lastArg %},{%endif%} {%endeach%} - ); + ); {%if return.isResultOrError %} - baton->error_code = result; - if (result < GIT_OK && giterr_last() != NULL) { - baton->error = git_error_dup(giterr_last()); - } + baton->error_code = result; + if (result < GIT_OK && giterr_last() != NULL) { + baton->error = git_error_dup(giterr_last()); + } {%elsif return.isErrorCode %} - baton->error_code = result; + baton->error_code = result; - if (result != GIT_OK && giterr_last() != NULL) { - baton->error = git_error_dup(giterr_last()); - } + if (result != GIT_OK && giterr_last() != NULL) { + baton->error = git_error_dup(giterr_last()); + } {%elsif not return.cType == 'void' %} - baton->result = result; + baton->result = result; {%endif%} } @@ -123,30 +128,32 @@ void {{ cppClassName }}::{{ cppFunctionName }}Worker::Execute() { void {{ cppClassName }}::{{ cppFunctionName }}Worker::HandleOKCallback() { {%if return.isResultOrError %} - if (baton->error_code >= GIT_OK) { + if (baton->error_code >= GIT_OK) { {%else%} - if (baton->error_code == GIT_OK) { + if (baton->error_code == GIT_OK) { {%endif%} - {%if return.isResultOrError %} + + {%if return.isResultOrError %} v8::Local result = Nan::New(baton->error_code); - {%elsif not .|returnsCount %} + {%elsif not .|returnsCount %} v8::Local result = Nan::Undefined(); - {%else%} + {%else%} v8::Local to; + {%if .|returnsCount > 1 %} + v8::Local result = Nan::New(); + {%endif%} + {%each .|returnsInfo 0 1 as _return %} + {%partial convertToV8 _return %} {%if .|returnsCount > 1 %} - v8::Local result = Nan::New(); - {%endif%} - {%each .|returnsInfo 0 1 as _return %} - {%partial convertToV8 _return %} - {%if .|returnsCount > 1 %} - Nan::Set(result, Nan::New("{{ _return.returnNameOrName }}").ToLocalChecked(), to); - {%endif%} - {%endeach%} - {%if .|returnsCount == 1 %} - v8::Local result = to; + Nan::Set(result, Nan::New("{{ _return.returnNameOrName }}").ToLocalChecked(), to); {%endif%} + {%endeach%} + {%if .|returnsCount == 1 %} + v8::Local result = to; {%endif%} + {%endif%} + v8::Local argv[2] = { Nan::Null(), result @@ -171,15 +178,15 @@ void {{ cppClassName }}::{{ cppFunctionName }}Worker::HandleOKCallback() { free((void *)baton->error); } else if (baton->error_code < 0) { std::queue< v8::Local > workerArguments; -{%each args|argsInfo as arg %} - {%if not arg.isReturn %} - {%if not arg.isSelf %} - {%if not arg.isCallbackFunction %} - workerArguments.push(GetFromPersistent("{{ arg.name }}")); - {%endif%} - {%endif%} - {%endif%} -{%endeach%} + {%each args|argsInfo as arg %} + {%if not arg.isReturn %} + {%if not arg.isSelf %} + {%if not arg.isCallbackFunction %} + workerArguments.push(GetFromPersistent("{{ arg.name }}")); + {%endif%} + {%endif%} + {%endif%} + {%endeach%} bool callbackFired = false; while(!workerArguments.empty()) { v8::Local node = workerArguments.front(); @@ -237,18 +244,18 @@ void {{ cppClassName }}::{{ cppFunctionName }}Worker::HandleOKCallback() { {%if arg.shouldAlloc %} {%if not arg.isCppClassStringOrArray %} {%elsif arg | isOid %} - if (baton->{{ arg.name}}NeedsFree) { - baton->{{ arg.name}}NeedsFree = false; - free((void*)baton->{{ arg.name }}); - } + if (baton->{{ arg.name}}NeedsFree) { + baton->{{ arg.name}}NeedsFree = false; + free((void*)baton->{{ arg.name }}); + } {%elsif arg.isCallbackFunction %} {%if not arg.payload.globalPayload %} - delete baton->{{ arg.payload.name }}; + delete baton->{{ arg.payload.name }}; {%endif%} {%elsif arg.globalPayload %} - delete ({{ cppFunctionName}}_globalPayload*)baton->{{ arg.name }}; + delete ({{ cppFunctionName}}_globalPayload*)baton->{{ arg.name }}; {%else%} - free((void*)baton->{{ arg.name }}); + free((void*)baton->{{ arg.name }}); {%endif%} {%endif%} {%endeach%} @@ -257,21 +264,21 @@ void {{ cppClassName }}::{{ cppFunctionName }}Worker::HandleOKCallback() { {%each args|argsInfo as arg %} {%if arg.isCppClassStringOrArray %} {%if arg.freeFunctionName %} - {{ arg.freeFunctionName }}(baton->{{ arg.name }}); + {{ arg.freeFunctionName }}(baton->{{ arg.name }}); {%elsif not arg.isConst%} - free((void *)baton->{{ arg.name }}); + free((void *)baton->{{ arg.name }}); {%endif%} {%elsif arg | isOid %} - if (baton->{{ arg.name}}NeedsFree) { - baton->{{ arg.name}}NeedsFree = false; - free((void *)baton->{{ arg.name }}); - } + if (baton->{{ arg.name}}NeedsFree) { + baton->{{ arg.name}}NeedsFree = false; + free((void *)baton->{{ arg.name }}); + } {%elsif arg.isCallbackFunction %} {%if not arg.payload.globalPayload %} - delete baton->{{ arg.payload.name }}; + delete baton->{{ arg.payload.name }}; {%endif%} {%elsif arg.globalPayload %} - delete ({{ cppFunctionName}}_globalPayload*)baton->{{ arg.name }}; + delete ({{ cppFunctionName}}_globalPayload*)baton->{{ arg.name }}; {%endif%} {%if arg.cppClassName == "GitBuf" %} {%if cppFunctionName == "Set" %} diff --git a/generate/templates/partials/sync_function.cc b/generate/templates/partials/sync_function.cc index c07c77b51..5f1306dae 100644 --- a/generate/templates/partials/sync_function.cc +++ b/generate/templates/partials/sync_function.cc @@ -6,9 +6,9 @@ NAN_METHOD({{ cppClassName }}::{{ cppFunctionName }}) { {%each .|returnsInfo 'true' as _return %} {%if _return.shouldAlloc %} - {{ _return.cType }}{{ _return.name }} = ({{ _return.cType }})malloc(sizeof({{ _return.cType|unPointer }})); + {{ _return.cType }}{{ _return.name }} = ({{ _return.cType }})malloc(sizeof({{ _return.cType|unPointer }})); {%else%} - {{ _return.cType|unPointer }} {{ _return.name }} = {{ _return.cType|unPointer|defaultValue }}; + {{ _return.cType|unPointer }} {{ _return.name }} = {{ _return.cType|unPointer|defaultValue }}; {%endif%} {%endeach%} @@ -17,112 +17,115 @@ NAN_METHOD({{ cppClassName }}::{{ cppFunctionName }}) { {%if not arg.isReturn %} {%partial convertFromV8 arg %} {%if arg.saveArg %} - v8::Local {{ arg.name }}(info[{{ arg.jsArg }}]->ToObject()); - {{ cppClassName }} *thisObj = Nan::ObjectWrap::Unwrap<{{ cppClassName }}>(info.This()); + v8::Local {{ arg.name }}(info[{{ arg.jsArg }}]->ToObject()); + {{ cppClassName }} *thisObj = Nan::ObjectWrap::Unwrap<{{ cppClassName }}>(info.This()); - thisObj->{{ cppFunctionName }}_{{ arg.name }}.Reset({{ arg.name }}); + thisObj->{{ cppFunctionName }}_{{ arg.name }}.Reset({{ arg.name }}); {%endif%} {%endif%} {%endif%} {%endeach%} -{%each args|argsInfo as arg %} -{%endeach%} - -{%-- Inside a free call, if the value is already free'd don't do it again.--%} -{% if cppFunctionName == "Free" %} -if (Nan::ObjectWrap::Unwrap<{{ cppClassName }}>(info.This())->GetValue() != NULL) { -{% endif %} + {%-- Inside a free call, if the value is already free'd don't do it again.--%} + {%if cppFunctionName == "Free" %} + if (Nan::ObjectWrap::Unwrap<{{ cppClassName }}>(info.This())->GetValue() != NULL) { + {%endif%} giterr_clear(); - { - LockMaster lockMaster(/*asyncAction: */false{%each args|argsInfo as arg %} - {%if arg.cType|isPointer%}{%if not arg.isReturn%} - ,{%if arg.isSelf %} - Nan::ObjectWrap::Unwrap<{{ arg.cppClassName }}>(info.This())->GetValue() - {%else%} - from_{{ arg.name }} - {%endif%} - {%endif%}{%endif%} - {%endeach%}); + { // lock master scope start + LockMaster lockMaster( + /*asyncAction: */false + {%each args|argsInfo as arg %} + {%if arg.cType|isPointer%} + {%if not arg.isReturn%} + , + {%if arg.isSelf %} + Nan::ObjectWrap::Unwrap<{{ arg.cppClassName }}>(info.This())->GetValue() + {%else%} + from_{{ arg.name }} + {%endif%} + {%endif%} + {%endif%} + {%endeach%} + ); - {%if .|hasReturnValue %} - {{ return.cType }} result = {%endif%}{{ cFunctionName }}( - {%each args|argsInfo as arg %} - {%if arg.isReturn %} - {%if not arg.shouldAlloc %}&{%endif%} - {%endif%} - {%if arg.isSelf %} - Nan::ObjectWrap::Unwrap<{{ arg.cppClassName }}>(info.This())->GetValue() - {%elsif arg.isReturn %} - {{ arg.name }} - {%else%} - from_{{ arg.name }} - {%endif%} - {%if not arg.lastArg %},{%endif%} - {%endeach%} + {%if .|hasReturnValue %} {{ return.cType }} result = {%endif%} + {{ cFunctionName }}( + {%each args|argsInfo as arg %} + {%if arg.isReturn %} + {%if not arg.shouldAlloc %}&{%endif%} + {%endif%} + {%if arg.isSelf %} + Nan::ObjectWrap::Unwrap<{{ arg.cppClassName }}>(info.This())->GetValue() + {%elsif arg.isReturn %} + {{ arg.name }} + {%else%} + from_{{ arg.name }} + {%endif%} + {%if not arg.lastArg %},{%endif%} + {%endeach%} ); - {%if .|hasReturnValue |and return.isErrorCode %} - if (result != GIT_OK) { + {%if .|hasReturnValue |and return.isErrorCode %} + if (result != GIT_OK) { + {%each args|argsInfo as arg %} + {%if arg.shouldAlloc %} + free({{ arg.name }}); + {%elsif arg | isOid %} + if (info[{{ arg.jsArg }}]->IsString()) { + free({{ arg.name }}); + } + {%endif%} + {%endeach%} + + if (giterr_last()) { + return Nan::ThrowError(giterr_last()->message); + } else { + return Nan::ThrowError("Unknown Error"); + } + } // lock master scope end + {%endif%} + + {%if cppFunctionName == "Free" %} + Nan::ObjectWrap::Unwrap<{{ cppClassName }}>(info.This())->ClearValue(); + } // lock master scope end + {%endif%} + + {%each args|argsInfo as arg %} - {%if arg.shouldAlloc %} - free({{ arg.name }}); - {%elsif arg | isOid %} + {%if arg | isOid %} if (info[{{ arg.jsArg }}]->IsString()) { - free({{ arg.name }}); + free((void *)from_{{ arg.name }}); } {%endif%} {%endeach%} - if (giterr_last()) { - return Nan::ThrowError(giterr_last()->message); - } else { - return Nan::ThrowError("Unknown Error"); - } - } - {%endif%} - - {% if cppFunctionName == "Free" %} - Nan::ObjectWrap::Unwrap<{{ cppClassName }}>(info.This())->ClearValue(); - } - {% endif %} - - - {%each args|argsInfo as arg %} - {%if arg | isOid %} - if (info[{{ arg.jsArg }}]->IsString()) { - free((void *)from_{{ arg.name }}); - } - {%endif%} - {%endeach%} - - {%if not .|returnsCount %} - return info.GetReturnValue().Set(scope.Escape(Nan::Undefined())); - {%else%} - {%if return.cType | isPointer %} - // null checks on pointers - if (!result) { + {%if not .|returnsCount %} return info.GetReturnValue().Set(scope.Escape(Nan::Undefined())); - } - {%endif%} + {%else%} + {%if return.cType | isPointer %} + // null checks on pointers + if (!result) { + return info.GetReturnValue().Set(scope.Escape(Nan::Undefined())); + } + {%endif%} - v8::Local to; - {%if .|returnsCount > 1 %} - v8::Local toReturn = Nan::New(); - {%endif%} - {%each .|returnsInfo as _return %} - {%partial convertToV8 _return %} + v8::Local to; {%if .|returnsCount > 1 %} - Nan::Set(toReturn, Nan::New("{{ _return.returnNameOrName }}").ToLocalChecked(), to); + v8::Local toReturn = Nan::New(); + {%endif%} + {%each .|returnsInfo as _return %} + {%partial convertToV8 _return %} + {%if .|returnsCount > 1 %} + Nan::Set(toReturn, Nan::New("{{ _return.returnNameOrName }}").ToLocalChecked(), to); + {%endif%} + {%endeach%} + {%if .|returnsCount == 1 %} + return info.GetReturnValue().Set(scope.Escape(to)); + {%else%} + return info.GetReturnValue().Set(scope.Escape(toReturn)); {%endif%} - {%endeach%} - {%if .|returnsCount == 1 %} - return info.GetReturnValue().Set(scope.Escape(to)); - {%else%} - return info.GetReturnValue().Set(scope.Escape(toReturn)); {%endif%} - {%endif%} } } From 6419fafa27d1128c64622f06cb50d370e0ca14cf Mon Sep 17 00:00:00 2001 From: Tyler Wanek Date: Fri, 12 Oct 2018 16:59:43 -0700 Subject: [PATCH 02/13] Enable setting arbitrary parameters as memory owner --- generate/templates/filters/fields_info.js | 1 + generate/templates/filters/returns_info.js | 4 +++- generate/templates/partials/convert_to_v8.cc | 15 ++++++++++++++- 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/generate/templates/filters/fields_info.js b/generate/templates/filters/fields_info.js index 8ec8a930b..0f04c6266 100644 --- a/generate/templates/filters/fields_info.js +++ b/generate/templates/filters/fields_info.js @@ -9,6 +9,7 @@ module.exports = function(fields) { fieldInfo.parsedName = field.name || "result"; fieldInfo.isCppClassIntType = ~["Uint32", "Int32"].indexOf(field.cppClassName); fieldInfo.parsedClassName = (field.cppClassName || '').toLowerCase() + "_t"; + fieldInfo.hasOwner = !!fieldInfo.ownedByThis; result.push(fieldInfo); }); diff --git a/generate/templates/filters/returns_info.js b/generate/templates/filters/returns_info.js index eeefa23c0..84b1508d9 100644 --- a/generate/templates/filters/returns_info.js +++ b/generate/templates/filters/returns_info.js @@ -34,7 +34,7 @@ module.exports = function(fn, argReturnsOnly, isAsync) { return_info.returnNameOrName = return_info.returnName || return_info.name; return_info.jsOrCppClassName = return_info.jsClassName || return_info.cppClassName; return_info.isOutParam = true; - return_info.hasOwner = return_info.ownedBy || return_info.ownedByThis; + return_info.hasOwner = !!(return_info.ownedBy || return_info.ownedByThis); return_info.ownedByIndex = -1; // Here we convert ownedBy, which is the name of the parameter @@ -57,6 +57,8 @@ module.exports = function(fn, argReturnsOnly, isAsync) { return_info.__proto__ = fn.return; return_info.isAsync = isAsync; + return_info.hasOwner = !!return_info.ownedByThis; + return_info.ownedByIndex = -1; return_info.parsedName = return_info.name && isAsync ? "baton->" + return_info.name : "result"; return_info.isCppClassIntType = ~['Uint32', 'Int32'].indexOf(return_info.cppClassName); return_info.parsedClassName = (return_info.cppClassName || '').toLowerCase() + "_t"; diff --git a/generate/templates/partials/convert_to_v8.cc b/generate/templates/partials/convert_to_v8.cc index 3d3bdf834..560ccc839 100644 --- a/generate/templates/partials/convert_to_v8.cc +++ b/generate/templates/partials/convert_to_v8.cc @@ -65,7 +65,20 @@ {% if cppClassName == 'Wrapper' %} to = {{ cppClassName }}::New({{= parsedName =}}); {% else %} - to = {{ cppClassName }}::New({{= parsedName =}}, {{ selfFreeing|toBool }} {% if ownedByThis %}, info.This(){% endif %}); + to = {{ cppClassName }}::New( + {{= parsedName =}}, + {{ selfFreeing|toBool }} + {% if hasOwner %} + , + {% if ownedByThis %} + info.This() + {% elsif isAsync %} + this->GetFromPersistent("{{= ownedBy =}}") + {% else %} + info[{{= ownedByIndex =}}]->ToObject() + {% endif %} + {% endif %} + ); {% endif %} } else { From 013518971bbc50f2471b2fedf9227afaf365006e Mon Sep 17 00:00:00 2001 From: Tyler Wanek Date: Tue, 16 Oct 2018 18:17:23 -0700 Subject: [PATCH 03/13] Update descriptor to handle more auto-gc behavior - Turns on selfFreeing for basically everything, including the repository - Ignores all git_*_free calls - Adds ownerFn info used to make associative ownership links between objects that have an exposed link to the repository - Added isSingleton flag for repository, this adds reference counting to the repo address externally so that no matter how many v8 objects point to it, only the last GC'd v8 object will free the repository. - Added an "ownedBy" field for arguments to enable ownership linking to arbitrary parameters, not just "this" - Some additional clean up of things that don't exist or shouldn't exist --- generate/input/descriptor.json | 328 +++++++++++++++++++++++++++++---- 1 file changed, 294 insertions(+), 34 deletions(-) diff --git a/generate/input/descriptor.json b/generate/input/descriptor.json index 35e6ee2b9..6b581c5fd 100644 --- a/generate/input/descriptor.json +++ b/generate/input/descriptor.json @@ -76,11 +76,35 @@ "types": { "annotated_commit": { + "selfFreeing": true, "functions": { + "git_annotated_commit_free": { + "ignore": true + }, "git_annotated_commit_id": { "return": { "ownedByThis": true } + }, + "git_annotated_commit_from_ref": { + "return": { + "ownedBy": "repo" + } + }, + "git_annotated_commit_from_fetchhead": { + "return": { + "ownedBy": "repo" + } + }, + "git_annotated_commit_lookup": { + "return": { + "ownedBy": "repo" + } + }, + "git_annotated_commit_from_revspec": { + "return": { + "ownedBy": "repo" + } } } }, @@ -115,14 +139,38 @@ } }, "blame": { + "selfFreeing": true, "cType": "git_blame", "functions": { + "git_blame_buffer": { + "args": { + "out": { + "ownedBy": "reference" + } + } + }, "git_blame_file": { "args": { + "out": { + "ownedBy": "repo" + }, "options": { "isOptional": true } } + }, + "git_blame_free": { + "ignore": true + }, + "git_blame_get_hunk_byindex": { + "return": { + "ownedByThis": true + } + }, + "git_blame_get_hunk_byline": { + "return": { + "ownedByThis": true + } } } }, @@ -135,6 +183,10 @@ }, "blob": { "selfFreeing": true, + "ownerFn": { + "name": "git_blob_owner", + "singletonCppClassName": "GitRepository" + }, "functions": { "git_blob_create_frombuffer": { "isAsync": true, @@ -176,6 +228,9 @@ "git_blob_create_fromchunks": { "ignore": true }, + "git_blob_dup": { + "ignore": true + }, "git_blob_filtered_content": { "isAsync": true, "isPrototypeMethod": false, @@ -206,6 +261,9 @@ "isErrorCode": true } }, + "git_blob_free": { + "ignore": true + }, "git_blob_id": { "return": { "ownedByThis": true @@ -314,6 +372,9 @@ }, "buf": { "functions": { + "git_buf_free": { + "ignore": true + }, "git_buf_grow": { "cppFunctionName": "Grow", "jsFunctionName": "grow", @@ -460,6 +521,10 @@ }, "commit": { "selfFreeing": true, + "ownerFn": { + "name": "git_commit_owner", + "singletonCppClassName": "GitRepository" + }, "functions": { "git_commit_amend": { "isAsync": true, @@ -555,9 +620,15 @@ "git_commit_create_from_v": { "ignore": true }, + "git_commit_dup": { + "ignore": true + }, "git_commit_extract_signature": { "isAsync": true }, + "git_commit_free": { + "ignore": true + }, "git_commit_id": { "return": { "ownedByThis": true @@ -589,6 +660,7 @@ } }, "config": { + "selfFreeing": true, "functions": { "git_config_add_backend": { "ignore": true @@ -657,7 +729,12 @@ "ignore": true }, "git_config_get_entry": { - "ignore": true + "args": { + "out": { + "isReturn": true, + "ownedBy": "cfg" + } + } }, "git_config_get_int32": { "ignore": true @@ -772,6 +849,9 @@ "isErrorCode": true } }, + "git_config_snapshot": { + "ignore": true + }, "git_config_find_programdata": { "isAsync": true, "return": { @@ -793,15 +873,22 @@ "config_backend": { "ignore": true }, + "config_entry": { + "selfFreeing": true + }, "config_iterator": { "ignore": true }, "cred": { + "selfFreeing": true, "cType": "git_cred", "functions": { "git_cred_default_new": { "isAsync": false }, + "git_cred_free": { + "ignore": true + }, "git_cred_ssh_custom_new": { "ignore": true }, @@ -851,6 +938,7 @@ "ignore": true }, "diff": { + "selfFreeing": true, "cDependencies": [ "git2/sys/diff.h" ], @@ -936,8 +1024,18 @@ "git_diff_get_stats": { "ignore": true }, + "git_diff_index_to_index": { + "args": { + "diff": { + "ownedBy": "repo" + } + } + }, "git_diff_index_to_workdir": { "args": { + "diff": { + "ownedBy": "repo" + }, "index": { "isOptional": true }, @@ -1008,6 +1106,9 @@ }, "git_diff_tree_to_index": { "args": { + "diff": { + "ownedBy": "repo" + }, "old_tree": { "isOptional": true }, @@ -1021,6 +1122,9 @@ }, "git_diff_tree_to_tree": { "args": { + "diff": { + "ownedBy": "repo" + }, "old_tree": { "isOptional": true }, @@ -1034,6 +1138,9 @@ }, "git_diff_tree_to_workdir": { "args": { + "diff": { + "ownedBy": "repo" + }, "old_tree": { "isOptional": true }, @@ -1044,6 +1151,9 @@ }, "git_diff_tree_to_workdir_with_index": { "args": { + "diff": { + "ownedBy": "repo" + }, "old_tree": { "isOptional": true }, @@ -1073,6 +1183,9 @@ "diff_similarity_metric": { "ignore": true }, + "diff_stats": { + "selfFreeing": true + }, "error_code": { "values": { "GIT_ERROR": { @@ -1145,7 +1258,8 @@ "args": { "filters": { "isReturn": true, - "cType": "git_filter_list **" + "cType": "git_filter_list **", + "ownedBy": "repo" }, "repo": { "cType": "git_repository *" @@ -1196,6 +1310,17 @@ "git2/sys/filter.h" ] }, + "filter_list": { + "selfFreeing": true, + "functions": { + "git_filter_list_free": { + "ignore": true + } + }, + "dependencies": [ + "../include/filter_registry.h" + ] + }, "graph": { "functions": { "git_graph_ahead_behind": { @@ -1223,6 +1348,12 @@ } }, "hashsig": { + "selfFreeing": true, + "functions": { + "git_hashsig_free": { + "ignore": true + } + }, "cDependencies": [ "git2/sys/hashsig.h" ] @@ -1244,6 +1375,11 @@ } }, "index": { + "selfFreeing": true, + "ownerFn": { + "name": "git_index_owner", + "singletonCppClassName": "GitRepository" + }, "functions": { "git_index_add": { "isAsync": true, @@ -1317,13 +1453,16 @@ "git_index_conflict_get": { "args": { "ancestor_out": { - "isReturn": true + "isReturn": true, + "ownedBy": "index" }, "our_out": { - "isReturn": true + "isReturn": true, + "ownedBy": "index" }, "their_out": { - "isReturn": true + "isReturn": true, + "ownedBy": "index" } }, "isAsync": true, @@ -1494,11 +1633,15 @@ "ignoreInit": true }, "indexer": { + "selfFreeing": true, "cType": "git_indexer", "functions": { "git_indexer_append": { "ignore": true }, + "git_indexer_free": { + "ignore": true + }, "git_indexer_hash": { "return": { "ownedByThis": true @@ -1613,6 +1756,7 @@ } }, "note": { + "selfFreeing": true, "functions": { "git_note_iterator_free": { "ignore": true @@ -1624,6 +1768,9 @@ } } }, + "git_note_free": { + "ignore": true + }, "git_note_id": { "return": { "ownedByThis": true @@ -1644,7 +1791,18 @@ } }, "object": { + "selfFreeing": true, + "ownerFn": { + "name": "git_object_owner", + "singletonCppClassName": "GitRepository" + }, "functions": { + "git_object_dup": { + "ignore": true + }, + "git_object_free": { + "ignore": true + }, "git_object_id": { "return": { "ownedByThis": true @@ -1660,6 +1818,7 @@ } }, "odb": { + "selfFreeing": true, "functions": { "git_odb_add_alternate": { "ignore": true @@ -1685,6 +1844,9 @@ "git_odb_foreach": { "ignore": true }, + "git_odb_free": { + "ignore": true + }, "git_odb_get_backend": { "ignore": true }, @@ -1762,6 +1924,9 @@ "jsClassName": "Buffer" } }, + "git_odb_object_free": { + "ignore": true + }, "git_odb_object_id": { "return": { "ownedByThis": true @@ -1832,16 +1997,31 @@ } } }, + "oid_shorten": { + "selfFreeing": true + }, + "oidarray": { + "selfFreeing": true, + "functions": { + "git_oidarray_free": { + "ignore": true + } + } + }, "openssl": { "cDependencies": [ "git2/sys/openssl.h" ] }, "packbuilder": { + "selfFreeing": true, "functions": { "git_packbuilder_foreach": { "ignore": true }, + "git_packbuilder_free": { + "ignore": true + }, "git_packbuilder_hash": { "return": { "ownedByThis": true @@ -1859,6 +2039,7 @@ } }, "patch": { + "selfFreeing": true, "dependencies": [ "../include/convenient_patch.h" ], @@ -1925,10 +2106,14 @@ } }, "pathspec": { + "selfFreeing": true, "dependencies": [ "../include/str_array_converter.h" ], "functions": { + "git_pathspec_free": { + "ignore": true + }, "git_pathspec_match_list_free": { "ignore": true }, @@ -1938,23 +2123,10 @@ } }, "push": { - "cType": "git_push", - "functions": { - "git_push_finish": { - "isAsync": true, - "return": { - "isErrorCode": true - } - }, - "git_push_set_callbacks": { - "ignore": true - }, - "git_push_status_foreach": { - "ignore": true - } - } + "ignore": true }, "rebase": { + "selfFreeing": true, "functions": { "git_rebase_commit": { "isAsync": true, @@ -2025,10 +2197,14 @@ } }, "refdb": { + "selfFreeing": true, "functions": { "git_refdb_backend_fs": { "ignore": true }, + "git_refdb_free": { + "ignore": true + }, "git_refdb_init_backend": { "ignore": true }, @@ -2046,6 +2222,10 @@ "reference": { "cppClassName": "GitRefs", "selfFreeing": true, + "ownerFn": { + "name": "git_reference_owner", + "singletonCppClassName": "GitRepository" + }, "functions": { "git_reference__alloc": { "ignore": true @@ -2123,7 +2303,16 @@ "ignore": true }, "reflog": { + "selfFreeing": true, "functions": { + "git_reflog_entry_byindex": { + "return": { + "ownedByThis": true + } + }, + "git_reflog_free": { + "ignore": true + }, "git_reflog_write": { "isAsync": true, "isSelf": true, @@ -2168,6 +2357,10 @@ ], "cType": "git_remote", "selfFreeing": true, + "ownerFn": { + "name": "git_remote_owner", + "singletonCppClassName": "GitRepository" + }, "functions": { "git_remote_create": { "isAsync": true, @@ -2231,6 +2424,9 @@ "isErrorCode": true } }, + "git_remote_free": { + "ignore": true + }, "git_remote_get_fetch_refspecs": { "args": { "array": { @@ -2340,10 +2536,22 @@ "selfFreeing": true }, "repository": { + "selfFreeing": true, + "isSingleton": true, "dependencies": [ "git2/sys/repository.h" ], "functions": { + "git_repository_config": { + "args": { + "out": { + "ownedByThis": true + } + } + }, + "git_repository_config_snapshot": { + "ignore": true + }, "git_repository_discover": { "isAsync": true, "return": { @@ -2366,6 +2574,9 @@ "isErrorCode": true } }, + "git_repository_free": { + "ignore": true + }, "git_repository_hashfile": { "ignore": true }, @@ -2498,6 +2709,10 @@ }, "revwalk": { "selfFreeing": true, + "ownerFn": { + "name": "git_revwalk_repository", + "singletonCppClassName": "GitRepository" + }, "dependencies": [ "../include/commit.h", "../include/functions/copy.h" @@ -2506,16 +2721,23 @@ "git_revwalk_add_hide_cb": { "ignore": true }, + "git_revwalk_free": { + "ignore": true + }, "git_revwalk_new": { "isAsync": false } } }, "signature": { + "selfFreeing": true, "dupFunction": "git_signature_dup", "functions": { "git_signature_default": { "isAsync": false + }, + "git_signature_free": { + "ignore": true }, "git_signature_new": { "isAsync": false @@ -2618,7 +2840,11 @@ } }, "status_list": { + "selfFreeing": true, "functions": { + "git_status_list_free": { + "ignore": true + }, "git_status_list_new": { "isAsync": true, "args": { @@ -2633,6 +2859,11 @@ } }, "strarray": { + "functions": { + "git_strarray_free": { + "ignore": true + } + }, "dependencies": [ "../include/str_array_converter.h" ] @@ -2644,6 +2875,11 @@ ] }, "submodule": { + "selfFreeing": true, + "ownerFn": { + "name": "git_submodule_owner", + "singletonCppClassName": "GitRepository" + }, "functions": { "git_submodule_add_to_index": { "isAsync": true, @@ -2682,6 +2918,9 @@ "type": "int" } }, + "git_submodule_free": { + "ignore": true + }, "git_submodule_index_id": { "return": { "ownedByThis": true @@ -2788,11 +3027,12 @@ "../include/str_array_converter.h" ], "selfFreeing": true, + "ownerFn": { + "name": "git_tag_owner", + "singletonCppClassName": "GitRepository" + }, "functions": { - "git_tag_foreach": { - "ignore": true - }, - "git_tag_create": { + "git_tag_annotation_create": { "args": { "oid": { "isReturn": true @@ -2803,15 +3043,7 @@ }, "isAsync": true }, - "git_tag_create_frombuffer": { - "ignore": true - }, - "git_tag_id": { - "return": { - "ownedByThis": true - } - }, - "git_tag_create_lightweight": { + "git_tag_create": { "args": { "oid": { "isReturn": true @@ -2822,7 +3054,10 @@ }, "isAsync": true }, - "git_tag_annotation_create": { + "git_tag_create_frombuffer": { + "ignore": true + }, + "git_tag_create_lightweight": { "args": { "oid": { "isReturn": true @@ -2833,6 +3068,17 @@ }, "isAsync": true }, + "git_tag_foreach": { + "ignore": true + }, + "git_tag_free": { + "ignore": true + }, + "git_tag_id": { + "return": { + "ownedByThis": true + } + }, "git_tag_list": { "args": { "tag_names": { @@ -2957,6 +3203,10 @@ }, "tree": { "selfFreeing": true, + "ownerFn": { + "name": "git_tree_owner", + "singletonCppClassName": "GitRepository" + }, "functions": { "git_tree_entry_byid": { "return": { @@ -2981,6 +3231,9 @@ "git_tree_entrycount": { "jsFunctionName": "entryCount" }, + "git_tree_free": { + "ignore": true + }, "git_tree_id": { "return": { "ownedByThis": true @@ -2992,10 +3245,14 @@ } }, "treebuilder": { + "selfFreeing": true, "functions": { "git_treebuilder_filter": { "ignore": true }, + "git_treebuilder_free": { + "ignore": true + }, "git_treebuilder_get": { "return": { "selfFreeing": false, @@ -3039,6 +3296,9 @@ "dupFunction": "git_tree_entry_dup", "freeFunctionName": "git_tree_entry_free", "functions": { + "git_tree_entry_free": { + "ignore": true + }, "git_tree_entry_id": { "return": { "ownedByThis": true From 2e5451fcd0489f7d92ca8f5c74813e86569fd58b Mon Sep 17 00:00:00 2001 From: Tyler Wanek Date: Tue, 16 Oct 2018 18:20:20 -0700 Subject: [PATCH 04/13] Add additional GC behavior that reflects new descriptor format - Implements isSingleton behavior with reference counting - Implements ownerFn, which ties an arbitrary pointer to a singleton type pointer - always calls free, even if object is "owned", the ownership link is about keeping internal pointers alive, but it seems that most calls to free are reference counted in libgit2, but the repository has a different relationship than all other types of ref counted frees. --- generate/templates/filters/returns_info.js | 15 ++++++++-- .../manual/include/nodegit_wrapper.h | 1 + .../manual/include/reference_counter.h | 30 +++++++++++++++++++ .../templates/manual/src/nodegit_wrapper.cc | 25 +++++++++++----- .../templates/manual/src/reference_counter.cc | 24 +++++++++++++++ generate/templates/partials/convert_to_v8.cc | 11 +++++-- generate/templates/partials/traits.h | 9 +++++- generate/templates/templates/binding.gyp | 1 + generate/templates/templates/class_header.h | 2 ++ generate/templates/templates/struct_header.h | 2 ++ 10 files changed, 106 insertions(+), 14 deletions(-) create mode 100644 generate/templates/manual/include/reference_counter.h create mode 100644 generate/templates/manual/src/reference_counter.cc diff --git a/generate/templates/filters/returns_info.js b/generate/templates/filters/returns_info.js index 84b1508d9..1c8f22387 100644 --- a/generate/templates/filters/returns_info.js +++ b/generate/templates/filters/returns_info.js @@ -12,8 +12,12 @@ module.exports = function(fn, argReturnsOnly, isAsync) { // any sort of string to argument index // in the template. var nameToArgIndex = {}; + var thisArgName = ''; args.forEach(function (arg, index) { nameToArgIndex[arg.name] = index; + if (arg.isSelf) { + thisArgName = arg.name; + } }); args.forEach(function (arg) { @@ -34,9 +38,13 @@ module.exports = function(fn, argReturnsOnly, isAsync) { return_info.returnNameOrName = return_info.returnName || return_info.name; return_info.jsOrCppClassName = return_info.jsClassName || return_info.cppClassName; return_info.isOutParam = true; - return_info.hasOwner = !!(return_info.ownedBy || return_info.ownedByThis); + return_info.hasOwner = !!(return_info.ownedBy || return_info.ownedByThis || return_info.ownerFn); return_info.ownedByIndex = -1; + if (return_info.ownedByThis) { + return_info.ownedBy = thisArgName; + } + // Here we convert ownedBy, which is the name of the parameter // that owns this result to the argument index. // sync functions will need to know this. @@ -57,7 +65,10 @@ module.exports = function(fn, argReturnsOnly, isAsync) { return_info.__proto__ = fn.return; return_info.isAsync = isAsync; - return_info.hasOwner = !!return_info.ownedByThis; + return_info.hasOwner = !!(return_info.ownedByThis || return_info.ownerFn); + if (return_info.ownedByThis) { + return_info.ownedBy = thisArgName; + } return_info.ownedByIndex = -1; return_info.parsedName = return_info.name && isAsync ? "baton->" + return_info.name : "result"; return_info.isCppClassIntType = ~['Uint32', 'Int32'].indexOf(return_info.cppClassName); diff --git a/generate/templates/manual/include/nodegit_wrapper.h b/generate/templates/manual/include/nodegit_wrapper.h index ea5277a50..c40b7af1d 100644 --- a/generate/templates/manual/include/nodegit_wrapper.h +++ b/generate/templates/manual/include/nodegit_wrapper.h @@ -2,6 +2,7 @@ #define NODEGIT_WRAPPER_H #include +#include // the Traits template parameter supplies: // typename cppClass - the C++ type of the NodeGit wrapper (e.g. GitRepository) diff --git a/generate/templates/manual/include/reference_counter.h b/generate/templates/manual/include/reference_counter.h new file mode 100644 index 000000000..2afa2310c --- /dev/null +++ b/generate/templates/manual/include/reference_counter.h @@ -0,0 +1,30 @@ +#ifndef REFERENCE_COUNTER_H +#define REFERENCE_COUNTER_H + +#include + +#include "lock_master.h" + +// There are certain instances in libgit2 which can be retrieved from multiple sources +// We need to make sure that we're counting how many times we've seen that pointer +// so that when we are performing free behavior, we don't free it until it is no longer +// referenced. The main example of this behavior is the repository instance, where +// after git_repository_open open (first instance) we can git_commit_lookup, followed by +// git_commit_owner (second instance). +// +// I was hoping that we could construct a Persistent handle, but that would interfere with +// GC. We want it to attmept to GC, and if this handle exists, the final repo will not +// free itself :(. +// +// Make sure to utilize LockMaster when incrementing or decrementing a reference count. +class ReferenceCounter { +public: + static void incrementCountForPointer(void *ptr); + + static unsigned long decrementCountForPointer(void *ptr); + +private: + static std::unordered_map referenceCountByPointer; +}; + +#endif diff --git a/generate/templates/manual/src/nodegit_wrapper.cc b/generate/templates/manual/src/nodegit_wrapper.cc index ea6694425..26ead60da 100644 --- a/generate/templates/manual/src/nodegit_wrapper.cc +++ b/generate/templates/manual/src/nodegit_wrapper.cc @@ -1,17 +1,26 @@ template NodeGitWrapper::NodeGitWrapper(typename Traits::cType *raw, bool selfFreeing, v8::Local owner) { - if (!owner.IsEmpty()) { - // if we have an owner, there are two options - either we duplicate the raw object - // (so we own the duplicate, and can self-free it) - // or we keep a handle on the owner so it doesn't get garbage collected - // while this wrapper is accessible - if(Traits::isDuplicable) { + if (Traits::isSingleton) { + ReferenceCounter::incrementCountForPointer((void *)raw); + this->raw = raw; + } else if (!owner.IsEmpty()) { + // if we have an owner, it could mean 2 things: + // 1. We are borrowed memory from another struct and should not be freed. We will keep a handle to the owner + // so that the owner isn't gc'd while we are using its memory. + // 2. We are borrowed memory from another struct and can be duplicated, so we should duplicate + // and become selfFreeing. + // 3. We are cached memory, potentially on the repo or config. + // Even though we have a handle in another objects cache, we are expected to call free, + // otherwise we are leaking memory. Cached objects are reference counted in libgit2, but will be leaked + // even if the cache is cleared if we haven't freed them. We will keep a handle on the owner, even though it + // is probably safe as we're reference counted. This should at worst just ensure that the cache owner is the + // last thing to be freed, and that is more safety than anything else. + if (Traits::isDuplicable) { Traits::duplicate(&this->raw, raw); selfFreeing = true; } else { this->owner.Reset(owner); this->raw = raw; - selfFreeing = false; } } else { this->raw = raw; @@ -34,7 +43,7 @@ NodeGitWrapper::NodeGitWrapper(const char *error) { template NodeGitWrapper::~NodeGitWrapper() { - if(Traits::isFreeable && selfFreeing) { + if (Traits::isFreeable && selfFreeing) { Traits::free(raw); SelfFreeingInstanceCount--; raw = NULL; diff --git a/generate/templates/manual/src/reference_counter.cc b/generate/templates/manual/src/reference_counter.cc new file mode 100644 index 000000000..1adc1df4b --- /dev/null +++ b/generate/templates/manual/src/reference_counter.cc @@ -0,0 +1,24 @@ +#include "../include/reference_counter.h" + +void ReferenceCounter::incrementCountForPointer(void *ptr) { + LockMaster(true, &referenceCountByPointer); + if (referenceCountByPointer.find(ptr) == referenceCountByPointer.end()) { + referenceCountByPointer[ptr] = 1; + } else { + referenceCountByPointer[ptr] = referenceCountByPointer[ptr] + 1; + } +} + +unsigned long ReferenceCounter::decrementCountForPointer(void *ptr) { + LockMaster(true, &referenceCountByPointer); + unsigned long referenceCount = referenceCountByPointer[ptr]; + if (referenceCount == 1) { + referenceCountByPointer.erase(ptr); + return 0; + } else { + referenceCountByPointer[ptr] = referenceCount - 1; + return referenceCountByPointer[ptr]; + } +} + +std::unordered_map ReferenceCounter::referenceCountByPointer; diff --git a/generate/templates/partials/convert_to_v8.cc b/generate/templates/partials/convert_to_v8.cc index 560ccc839..fd3de20c4 100644 --- a/generate/templates/partials/convert_to_v8.cc +++ b/generate/templates/partials/convert_to_v8.cc @@ -70,10 +70,15 @@ {{ selfFreeing|toBool }} {% if hasOwner %} , - {% if ownedByThis %} - info.This() + {% if ownerFn | toBool %} + {{= ownerFn.singletonCppClassName =}}::New( + {{= ownerFn.name =}}({{= parsedName =}}), + true + )->ToObject() {% elsif isAsync %} - this->GetFromPersistent("{{= ownedBy =}}") + this->GetFromPersistent("{{= ownedBy =}}")->ToObject() + {% elsif ownedByThis %} + info.This() {% else %} info[{{= ownedByIndex =}}]->ToObject() {% endif %} diff --git a/generate/templates/partials/traits.h b/generate/templates/partials/traits.h index efcc4b49a..3e63e42e8 100644 --- a/generate/templates/partials/traits.h +++ b/generate/templates/partials/traits.h @@ -17,10 +17,17 @@ struct {{ cppClassName }}Traits { {% endif %} } + static const bool isSingleton = {{ isSingleton | toBool }}; static const bool isFreeable = {{ freeFunctionName | toBool}}; static void free({{ cType }} *raw) { {% if freeFunctionName %} - ::{{ freeFunctionName }}(raw); // :: to avoid calling this free recursively + unsigned long referenceCount = 0; + {% if isSingleton %} + referenceCount = ReferenceCounter::decrementCountForPointer((void *)raw); + {% endif %} + if (referenceCount == 0) { + ::{{ freeFunctionName }}(raw); // :: to avoid calling this free recursively + } {% else %} Nan::ThrowError("free called on {{ cppClassName }} which cannot be freed"); {% endif %} diff --git a/generate/templates/templates/binding.gyp b/generate/templates/templates/binding.gyp index a332fc74c..e9b2ab17b 100644 --- a/generate/templates/templates/binding.gyp +++ b/generate/templates/templates/binding.gyp @@ -75,6 +75,7 @@ "sources": [ "src/async_baton.cc", "src/lock_master.cc", + "src/reference_counter.cc", "src/nodegit.cc", "src/init_ssh2.cc", "src/promise_completion.cc", diff --git a/generate/templates/templates/class_header.h b/generate/templates/templates/class_header.h index ea04c120f..fce17e58c 100644 --- a/generate/templates/templates/class_header.h +++ b/generate/templates/templates/class_header.h @@ -4,10 +4,12 @@ #include #include #include +#include #include "async_baton.h" #include "nodegit_wrapper.h" #include "promise_completion.h" +#include "reference_counter.h" extern "C" { #include diff --git a/generate/templates/templates/struct_header.h b/generate/templates/templates/struct_header.h index 4d2ec2af5..568bcfc91 100644 --- a/generate/templates/templates/struct_header.h +++ b/generate/templates/templates/struct_header.h @@ -4,9 +4,11 @@ #include #include #include +#include #include "async_baton.h" #include "callback_wrapper.h" +#include "reference_counter.h" #include "nodegit_wrapper.h" extern "C" { From a2388e3ce4e990dd76229845323c7f5b5344187d Mon Sep 17 00:00:00 2001 From: Tyler Wanek Date: Tue, 16 Oct 2018 18:21:23 -0700 Subject: [PATCH 05/13] git_filter_list has a complex owner scenario and requires adjustments I know we will need this in the future for merge drivers and the like. But for now, this will at least show how it could be done. --- generate/input/libgit2-supplement.json | 7 + generate/templates/manual/filter_list/load.cc | 233 ++++++++++++++++++ 2 files changed, 240 insertions(+) create mode 100644 generate/templates/manual/filter_list/load.cc diff --git a/generate/input/libgit2-supplement.json b/generate/input/libgit2-supplement.json index e1fb11d3b..064a9d02e 100644 --- a/generate/input/libgit2-supplement.json +++ b/generate/input/libgit2-supplement.json @@ -144,6 +144,13 @@ "isErrorCode": true } }, + "git_filter_list_load": { + "isManual": true, + "cFile": "generate/templates/manual/filter_list/load.cc", + "isAsync": true, + "isPrototypeMethod": false, + "group": "filter_list" + }, "git_patch_convenient_from_diff": { "args": [ { diff --git a/generate/templates/manual/filter_list/load.cc b/generate/templates/manual/filter_list/load.cc new file mode 100644 index 000000000..6c24fa1b2 --- /dev/null +++ b/generate/templates/manual/filter_list/load.cc @@ -0,0 +1,233 @@ +/* + * @param Repository repo + * @param Blob blob + * @param String path + * @param Number mode + * @param Number flags + * @param FilterList callback + */ +NAN_METHOD(GitFilterList::Load) { + if (info.Length() == 0 || !info[0]->IsObject()) { + return Nan::ThrowError("Repository repo is required."); + } + + if (info.Length() == 2 || !info[2]->IsString()) { + return Nan::ThrowError("String path is required."); + } + + if (info.Length() == 3 || !info[3]->IsNumber()) { + return Nan::ThrowError("Number mode is required."); + } + + if (info.Length() == 4 || !info[4]->IsNumber()) { + return Nan::ThrowError("Number flags is required."); + } + + if (info.Length() == 5 || !info[5]->IsFunction()) { + return Nan::ThrowError("Callback is required and must be a Function."); + } + + LoadBaton *baton = new LoadBaton; + + baton->error_code = GIT_OK; + baton->error = NULL; + + // start convert_from_v8 block + git_repository *from_repo = NULL; + from_repo = + Nan::ObjectWrap::Unwrap(info[0]->ToObject())->GetValue(); + // end convert_from_v8 block + baton->repo = from_repo; + // start convert_from_v8 block + git_blob *from_blob = NULL; + if (info[1]->IsObject()) { + from_blob = + Nan::ObjectWrap::Unwrap(info[1]->ToObject())->GetValue(); + } else { + from_blob = 0; + } + // end convert_from_v8 block + baton->blob = from_blob; + // start convert_from_v8 block + const char *from_path = NULL; + + String::Utf8Value path(info[2]->ToString()); + // malloc with one extra byte so we can add the terminating null character + // C-strings expect: + from_path = (const char *)malloc(path.length() + 1); + // copy the characters from the nodejs string into our C-string (used instead + // of strdup or strcpy because nulls in the middle of strings are valid coming + // from nodejs): + memcpy((void *)from_path, *path, path.length()); + // ensure the final byte of our new string is null, extra casts added to + // ensure compatibility with various C types used in the nodejs binding + // generation: + memset((void *)(((char *)from_path) + path.length()), 0, 1); + // end convert_from_v8 block + baton->path = from_path; + // start convert_from_v8 block + git_filter_mode_t from_mode; + from_mode = (git_filter_mode_t)(int)info[3].As()->Value(); + // end convert_from_v8 block + baton->mode = from_mode; + // start convert_from_v8 block + uint32_t from_flags; + from_flags = (uint32_t)info[4].As()->Value(); + // end convert_from_v8 block + baton->flags = from_flags; + + Nan::Callback *callback = + new Nan::Callback(v8::Local::Cast(info[5])); + LoadWorker *worker = new LoadWorker(baton, callback); + + if (!info[0]->IsUndefined() && !info[0]->IsNull()) + worker->SaveToPersistent("repo", info[0]->ToObject()); + if (!info[1]->IsUndefined() && !info[1]->IsNull()) + worker->SaveToPersistent("blob", info[1]->ToObject()); + if (!info[2]->IsUndefined() && !info[2]->IsNull()) + worker->SaveToPersistent("path", info[2]->ToObject()); + if (!info[3]->IsUndefined() && !info[3]->IsNull()) + worker->SaveToPersistent("mode", info[3]->ToObject()); + if (!info[4]->IsUndefined() && !info[4]->IsNull()) + worker->SaveToPersistent("flags", info[4]->ToObject()); + + AsyncLibgit2QueueWorker(worker); + return; +} + +void GitFilterList::LoadWorker::Execute() { + giterr_clear(); + + { + LockMaster lockMaster( + /*asyncAction: */ true, baton->repo, baton->blob, baton->path); + + int result = git_filter_list_load(&baton->filters, baton->repo, baton->blob, + baton->path, baton->mode, baton->flags); + + baton->error_code = result; + + if (result != GIT_OK && giterr_last() != NULL) { + baton->error = git_error_dup(giterr_last()); + } + } +} + +void GitFilterList::LoadWorker::HandleOKCallback() { + if (baton->error_code == GIT_OK) { + v8::Local to; + // start convert_to_v8 block + + if (baton->filters != NULL) { + // GitFilterList baton->filters + v8::Local owners = Nan::New(0); + v8::Local filterRegistry = Nan::New(GitFilterRegistry::persistentHandle); + v8::Local propertyNames = filterRegistry->GetPropertyNames(); + + Nan::Set( + owners, + Nan::New(0), + this->GetFromPersistent("repo")->ToObject() + ); + + for (uint32_t index = 0; index < propertyNames->Length(); ++index) { + v8::Local propertyName = propertyNames->Get(index)->ToString(); + String::Utf8Value propertyNameAsUtf8Value(propertyName); + const char *propertyNameAsCString = *propertyNameAsUtf8Value; + + bool isNotMethodOnRegistry = strcmp("register", propertyNameAsCString) + && strcmp("unregister", propertyNameAsCString); + if (isNotMethodOnRegistry && git_filter_list_contains(baton->filters, propertyNameAsCString)) { + Nan::Set( + owners, + Nan::New(owners->Length()), + filterRegistry->Get(propertyName) + ); + } + } + + to = GitFilterList::New(baton->filters, true, owners->ToObject()); + } else { + to = Nan::Null(); + } + + // end convert_to_v8 block + v8::Local result = to; + + v8::Local argv[2] = {Nan::Null(), result}; + callback->Call(2, argv, async_resource); + } else { + if (baton->error) { + v8::Local err; + if (baton->error->message) { + err = Nan::Error(baton->error->message)->ToObject(); + } else { + err = Nan::Error("Method load has thrown an error.")->ToObject(); + } + err->Set(Nan::New("errno").ToLocalChecked(), Nan::New(baton->error_code)); + err->Set(Nan::New("errorFunction").ToLocalChecked(), + Nan::New("FilterList.load").ToLocalChecked()); + v8::Local argv[1] = {err}; + callback->Call(1, argv, async_resource); + if (baton->error->message) + free((void *)baton->error->message); + free((void *)baton->error); + } else if (baton->error_code < 0) { + std::queue> workerArguments; + workerArguments.push(GetFromPersistent("repo")); + workerArguments.push(GetFromPersistent("blob")); + workerArguments.push(GetFromPersistent("path")); + workerArguments.push(GetFromPersistent("mode")); + workerArguments.push(GetFromPersistent("flags")); + bool callbackFired = false; + while (!workerArguments.empty()) { + v8::Local node = workerArguments.front(); + workerArguments.pop(); + + if (!node->IsObject() || node->IsArray() || node->IsBooleanObject() || + node->IsDate() || node->IsFunction() || node->IsNumberObject() || + node->IsRegExp() || node->IsStringObject()) { + continue; + } + + v8::Local nodeObj = node->ToObject(); + v8::Local checkValue = GetPrivate( + nodeObj, Nan::New("NodeGitPromiseError").ToLocalChecked()); + + if (!checkValue.IsEmpty() && !checkValue->IsNull() && + !checkValue->IsUndefined()) { + v8::Local argv[1] = {checkValue->ToObject()}; + callback->Call(1, argv, async_resource); + callbackFired = true; + break; + } + + v8::Local properties = nodeObj->GetPropertyNames(); + for (unsigned int propIndex = 0; propIndex < properties->Length(); + ++propIndex) { + v8::Local propName = + properties->Get(propIndex)->ToString(); + v8::Local nodeToQueue = nodeObj->Get(propName); + if (!nodeToQueue->IsUndefined()) { + workerArguments.push(nodeToQueue); + } + } + } + + if (!callbackFired) { + v8::Local err = + Nan::Error("Method load has thrown an error.")->ToObject(); + err->Set(Nan::New("errno").ToLocalChecked(), + Nan::New(baton->error_code)); + err->Set(Nan::New("errorFunction").ToLocalChecked(), + Nan::New("FilterList.load").ToLocalChecked()); + v8::Local argv[1] = {err}; + callback->Call(1, argv, async_resource); + } + } else { + callback->Call(0, NULL, async_resource); + } + } + + delete baton; +} From 8c5d58b3dea40ba0baf82616beeb355e60829c76 Mon Sep 17 00:00:00 2001 From: Tyler Wanek Date: Wed, 17 Oct 2018 09:16:44 -0700 Subject: [PATCH 06/13] Move this inherited clone behavior into C++ land It looks like this is pretty important to preserve, as it broke tests on windows immediately when removing it. Because we are no longer exposing free methods in javascript land, we will need to manually perform this method at the C++ layer. --- generate/input/libgit2-supplement.json | 7 + generate/templates/manual/clone/clone.cc | 211 ++++++++++++++++++ generate/templates/manual/filter_list/load.cc | 12 + lib/clone.js | 16 +- 4 files changed, 231 insertions(+), 15 deletions(-) create mode 100644 generate/templates/manual/clone/clone.cc diff --git a/generate/input/libgit2-supplement.json b/generate/input/libgit2-supplement.json index 064a9d02e..0c6e22a50 100644 --- a/generate/input/libgit2-supplement.json +++ b/generate/input/libgit2-supplement.json @@ -110,6 +110,13 @@ }, "group": "branch" }, + "git_clone": { + "isManual": true, + "cFile": "generate/templates/manual/clone/clone.cc", + "isAsync": true, + "isPrototypeMethod": false, + "group": "clone" + }, "git_commit_extract_signature": { "args": [ { diff --git a/generate/templates/manual/clone/clone.cc b/generate/templates/manual/clone/clone.cc new file mode 100644 index 000000000..7ddcd559b --- /dev/null +++ b/generate/templates/manual/clone/clone.cc @@ -0,0 +1,211 @@ +// NOTE you may need to occasionally rebuild this method by calling the generators +// if major changes are made to the templates / generator. + +// Due to some file locking issues, we have the need to free a repository after it's cloned. +// We do not expose free functions to javascript, and so, we've moved the implementation of +// cloning, freeing the repo, and opening the repo into a custom template. + +/* + * @param String url + * @param String local_path + * @param CloneOptions options + * @param Repository callback + */ +NAN_METHOD(GitClone::Clone) { + + if (info.Length() == 0 || !info[0]->IsString()) { + return Nan::ThrowError("String url is required."); + } + + if (info.Length() == 1 || !info[1]->IsString()) { + return Nan::ThrowError("String local_path is required."); + } + + if (info.Length() == 3 || !info[3]->IsFunction()) { + return Nan::ThrowError("Callback is required and must be a Function."); + } + + CloneBaton *baton = new CloneBaton; + + baton->error_code = GIT_OK; + baton->error = NULL; + + // start convert_from_v8 block + const char *from_url = NULL; + + String::Utf8Value url(info[0]->ToString()); + // malloc with one extra byte so we can add the terminating null character + // C-strings expect: + from_url = (const char *)malloc(url.length() + 1); + // copy the characters from the nodejs string into our C-string (used instead + // of strdup or strcpy because nulls in the middle of strings are valid coming + // from nodejs): + memcpy((void *)from_url, *url, url.length()); + // ensure the final byte of our new string is null, extra casts added to + // ensure compatibility with various C types used in the nodejs binding + // generation: + memset((void *)(((char *)from_url) + url.length()), 0, 1); + // end convert_from_v8 block + baton->url = from_url; + // start convert_from_v8 block + const char *from_local_path = NULL; + + String::Utf8Value local_path(info[1]->ToString()); + // malloc with one extra byte so we can add the terminating null character + // C-strings expect: + from_local_path = (const char *)malloc(local_path.length() + 1); + // copy the characters from the nodejs string into our C-string (used instead + // of strdup or strcpy because nulls in the middle of strings are valid coming + // from nodejs): + memcpy((void *)from_local_path, *local_path, local_path.length()); + // ensure the final byte of our new string is null, extra casts added to + // ensure compatibility with various C types used in the nodejs binding + // generation: + memset((void *)(((char *)from_local_path) + local_path.length()), 0, 1); + // end convert_from_v8 block + baton->local_path = from_local_path; + // start convert_from_v8 block + const git_clone_options *from_options = NULL; + if (info[2]->IsObject()) { + from_options = Nan::ObjectWrap::Unwrap(info[2]->ToObject()) + ->GetValue(); + } else { + from_options = 0; + } + // end convert_from_v8 block + baton->options = from_options; + + Nan::Callback *callback = + new Nan::Callback(v8::Local::Cast(info[3])); + CloneWorker *worker = new CloneWorker(baton, callback); + + if (!info[0]->IsUndefined() && !info[0]->IsNull()) + worker->SaveToPersistent("url", info[0]->ToObject()); + if (!info[1]->IsUndefined() && !info[1]->IsNull()) + worker->SaveToPersistent("local_path", info[1]->ToObject()); + if (!info[2]->IsUndefined() && !info[2]->IsNull()) + worker->SaveToPersistent("options", info[2]->ToObject()); + + AsyncLibgit2QueueWorker(worker); + return; +} + +void GitClone::CloneWorker::Execute() { + giterr_clear(); + + { + LockMaster lockMaster( + /*asyncAction: */ true, baton->url, baton->local_path, baton->options); + + git_repository *repo; + int result = + git_clone(&repo, baton->url, baton->local_path, baton->options); + + if (result == GIT_OK) { + // This is required to clean up after the clone to avoid file locking + // issues in Windows and potentially other issues we don't know about. + git_repository_free(repo); + + // We want to provide a valid repository object, so reopen the repository + // after clone and cleanup. + result = git_repository_open(&baton->out, baton->local_path); + } + + baton->error_code = result; + + if (result != GIT_OK && giterr_last() != NULL) { + baton->error = git_error_dup(giterr_last()); + } + } +} + +void GitClone::CloneWorker::HandleOKCallback() { + if (baton->error_code == GIT_OK) { + v8::Local to; + // start convert_to_v8 block + + if (baton->out != NULL) { + // GitRepository baton->out + to = GitRepository::New(baton->out, true); + } else { + to = Nan::Null(); + } + + // end convert_to_v8 block + v8::Local result = to; + + v8::Local argv[2] = {Nan::Null(), result}; + callback->Call(2, argv, async_resource); + } else { + if (baton->error) { + v8::Local err; + if (baton->error->message) { + err = Nan::Error(baton->error->message)->ToObject(); + } else { + err = Nan::Error("Method clone has thrown an error.")->ToObject(); + } + err->Set(Nan::New("errno").ToLocalChecked(), Nan::New(baton->error_code)); + err->Set(Nan::New("errorFunction").ToLocalChecked(), + Nan::New("Clone.clone").ToLocalChecked()); + v8::Local argv[1] = {err}; + callback->Call(1, argv, async_resource); + if (baton->error->message) + free((void *)baton->error->message); + free((void *)baton->error); + } else if (baton->error_code < 0) { + std::queue> workerArguments; + workerArguments.push(GetFromPersistent("url")); + workerArguments.push(GetFromPersistent("local_path")); + workerArguments.push(GetFromPersistent("options")); + bool callbackFired = false; + while (!workerArguments.empty()) { + v8::Local node = workerArguments.front(); + workerArguments.pop(); + + if (!node->IsObject() || node->IsArray() || node->IsBooleanObject() || + node->IsDate() || node->IsFunction() || node->IsNumberObject() || + node->IsRegExp() || node->IsStringObject()) { + continue; + } + + v8::Local nodeObj = node->ToObject(); + v8::Local checkValue = GetPrivate( + nodeObj, Nan::New("NodeGitPromiseError").ToLocalChecked()); + + if (!checkValue.IsEmpty() && !checkValue->IsNull() && + !checkValue->IsUndefined()) { + v8::Local argv[1] = {checkValue->ToObject()}; + callback->Call(1, argv, async_resource); + callbackFired = true; + break; + } + + v8::Local properties = nodeObj->GetPropertyNames(); + for (unsigned int propIndex = 0; propIndex < properties->Length(); + ++propIndex) { + v8::Local propName = + properties->Get(propIndex)->ToString(); + v8::Local nodeToQueue = nodeObj->Get(propName); + if (!nodeToQueue->IsUndefined()) { + workerArguments.push(nodeToQueue); + } + } + } + + if (!callbackFired) { + v8::Local err = + Nan::Error("Method clone has thrown an error.")->ToObject(); + err->Set(Nan::New("errno").ToLocalChecked(), + Nan::New(baton->error_code)); + err->Set(Nan::New("errorFunction").ToLocalChecked(), + Nan::New("Clone.clone").ToLocalChecked()); + v8::Local argv[1] = {err}; + callback->Call(1, argv, async_resource); + } + } else { + callback->Call(0, NULL, async_resource); + } + } + + delete baton; +} diff --git a/generate/templates/manual/filter_list/load.cc b/generate/templates/manual/filter_list/load.cc index 6c24fa1b2..1e7788e65 100644 --- a/generate/templates/manual/filter_list/load.cc +++ b/generate/templates/manual/filter_list/load.cc @@ -1,3 +1,15 @@ +// NOTE you may need to occasionally rebuild this method by calling the generators +// if major changes are made to the templates / generator. + +// git_filter_list_load has a more complex ownership pattern than is currently available +// in the generator. This is because it not only has to get the repo as an owner, +// but it also needs to discover which custom filters, a git_filter, it is bound to, if any. +// We must enforce that the custom filters are not freed before a git_filter_list is freed, +// but a git_filter_list also has pointers to the repo in it. + +// TODO In the future, it would be awesome if we could instead of writing a manual method like this, provide +// custom ownership methods that can be injected into the HandleOKCallback. + /* * @param Repository repo * @param Blob blob diff --git a/lib/clone.js b/lib/clone.js index d81e1a5da..c6b6be599 100644 --- a/lib/clone.js +++ b/lib/clone.js @@ -29,19 +29,5 @@ Clone.clone = function(url, local_path, options) { options.fetchOpts = fetchOpts; } - // This is required to clean up after the clone to avoid file locking - // issues in Windows and potentially other issues we don't know about. - var freeRepository = function(repository) { - repository.free(); - }; - - // We want to provide a valid repository object, so reopen the repository - // after clone and cleanup. - var openRepository = function() { - return NodeGit.Repository.open(local_path); - }; - - return _clone.call(this, url, local_path, options) - .then(freeRepository) - .then(openRepository); + return _clone.call(this, url, local_path, options); }; From 335cc016930db51c8603da3e0605d36be132301c Mon Sep 17 00:00:00 2001 From: Tyler Wanek Date: Wed, 17 Oct 2018 09:17:54 -0700 Subject: [PATCH 07/13] Fix tests --- lib/repository.js | 10 ---------- test/tests/annotated_commit.js | 19 ------------------- test/tests/filter.js | 3 --- test/tests/patch.js | 2 -- test/tests/repository.js | 5 +++++ 5 files changed, 5 insertions(+), 34 deletions(-) diff --git a/lib/repository.js b/lib/repository.js index 44bcd337d..d5e941134 100644 --- a/lib/repository.js +++ b/lib/repository.js @@ -803,11 +803,6 @@ Repository.prototype.discardLines = }) .then(function(content) { originalContent = content; - if (filterList) { - filterList.free(); - filterList = null; - } - return getPathHunks(repo, index, filePath, false, additionalDiffOptions); }) .then(function(hunks) { @@ -837,11 +832,6 @@ Repository.prototype.discardLines = }); }) .then(function(filteredContent) { - if (filterList) { - filterList.free(); - filterList = null; - } - return fse.writeFile(fullFilePath, filteredContent); }); }; diff --git a/test/tests/annotated_commit.js b/test/tests/annotated_commit.js index b702fcfba..c87ba859b 100644 --- a/test/tests/annotated_commit.js +++ b/test/tests/annotated_commit.js @@ -32,25 +32,6 @@ describe("AnnotatedCommit", function() { }); }); - it("can free an AnnotatedCommit after creating it", function() { - var test = this; - - return Branch.lookup(test.repository, branchName, Branch.BRANCH.LOCAL) - .then(function(ref) { - return AnnotatedCommit.fromRef(test.repository, ref); - }) - .then(function(annotatedCommit) { - // Annotated commit should exist - assert(annotatedCommit.id()); - - // Free the annotated commit - annotatedCommit.free(); - - // Getting the id should now throw because the commit was freed - assert.throws(annotatedCommit.id); - }); - }); - it("can lookup an AnnotatedCommit after creating it", function() { var test = this; var id; diff --git a/test/tests/filter.js b/test/tests/filter.js index 37a737c2c..3a57acbef 100644 --- a/test/tests/filter.js +++ b/test/tests/filter.js @@ -990,7 +990,6 @@ describe("Filter", function() { }) .then(function(content) { assert.equal(content, message); - list.free(); }); }); @@ -1036,7 +1035,6 @@ describe("Filter", function() { }) .then(function(content) { assert.equal(content, message); - list.free(); }); }); @@ -1093,7 +1091,6 @@ describe("Filter", function() { }) .then(function(content) { assert.equal(content, message); - list.free(); }); }); }); diff --git a/test/tests/patch.js b/test/tests/patch.js index e426d2b63..158685a08 100644 --- a/test/tests/patch.js +++ b/test/tests/patch.js @@ -61,6 +61,4 @@ describe("Patch", function() { }); }); - - }); diff --git a/test/tests/repository.js b/test/tests/repository.js index 0474568a4..fbe791ee7 100644 --- a/test/tests/repository.js +++ b/test/tests/repository.js @@ -326,4 +326,9 @@ describe("Repository", function() { assert.equal(numMergeHeads, 1); }); }); + + it("can GC", function() { + global.gc(); + assert.equal(true, true); + }); }); From 340e245045082288622f8013358f196849bb888c Mon Sep 17 00:00:00 2001 From: Steven King Jr Date: Wed, 17 Oct 2018 12:32:56 -0700 Subject: [PATCH 08/13] Fix Node 6 builds on Mac OS not having `unordered_map` defined Changes minimum support version of MacOS to 10.9. --- generate/templates/templates/binding.gyp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/generate/templates/templates/binding.gyp b/generate/templates/templates/binding.gyp index e9b2ab17b..9a0f8b7b7 100644 --- a/generate/templates/templates/binding.gyp +++ b/generate/templates/templates/binding.gyp @@ -134,7 +134,9 @@ ], "xcode_settings": { "GCC_ENABLE_CPP_EXCEPTIONS": "YES", - "MACOSX_DEPLOYMENT_TARGET": "10.7", + "MACOSX_DEPLOYMENT_TARGET": "10.9", + 'CLANG_CXX_LIBRARY': 'libc++', + 'CLANG_CXX_LANGUAGE_STANDARD':'c++11', "WARNING_CFLAGS": [ "-Wno-unused-variable", From d6858971b5c227f3c7c23a707f42079dab869808 Mon Sep 17 00:00:00 2001 From: Tyler Wanek Date: Fri, 19 Oct 2018 09:28:18 -0700 Subject: [PATCH 09/13] Improve ownership model co-authored-by: Steve King --- generate/templates/filters/fields_info.js | 2 +- generate/templates/filters/returns_info.js | 36 +++++++++----- generate/templates/partials/convert_to_v8.cc | 52 +++++++++++++------- 3 files changed, 60 insertions(+), 30 deletions(-) diff --git a/generate/templates/filters/fields_info.js b/generate/templates/filters/fields_info.js index 0f04c6266..7e1e1211b 100644 --- a/generate/templates/filters/fields_info.js +++ b/generate/templates/filters/fields_info.js @@ -9,7 +9,7 @@ module.exports = function(fields) { fieldInfo.parsedName = field.name || "result"; fieldInfo.isCppClassIntType = ~["Uint32", "Int32"].indexOf(field.cppClassName); fieldInfo.parsedClassName = (field.cppClassName || '').toLowerCase() + "_t"; - fieldInfo.hasOwner = !!fieldInfo.ownedByThis; + fieldInfo.hasOwner = !fieldInfo.selfOwned && !!fieldInfo.ownedByThis; result.push(fieldInfo); }); diff --git a/generate/templates/filters/returns_info.js b/generate/templates/filters/returns_info.js index 1c8f22387..2d178e2c1 100644 --- a/generate/templates/filters/returns_info.js +++ b/generate/templates/filters/returns_info.js @@ -38,18 +38,22 @@ module.exports = function(fn, argReturnsOnly, isAsync) { return_info.returnNameOrName = return_info.returnName || return_info.name; return_info.jsOrCppClassName = return_info.jsClassName || return_info.cppClassName; return_info.isOutParam = true; - return_info.hasOwner = !!(return_info.ownedBy || return_info.ownedByThis || return_info.ownerFn); - return_info.ownedByIndex = -1; + return_info.hasOwner = !return_info.selfOwned && + !!(return_info.ownedBy || return_info.ownedByThis || return_info.ownerFn); + return_info.ownedByIndices = []; - if (return_info.ownedByThis) { - return_info.ownedBy = thisArgName; + if (isAsync && return_info.ownedByThis) { + return_info.ownedBy = return_info.ownedBy || []; + return_info.ownedBy.unshift(thisArgName); } - // Here we convert ownedBy, which is the name of the parameter - // that owns this result to the argument index. + // Here we convert ownedBy, which is the list of parameter names + // that own this result to the argument index. // sync functions will need to know this. if (!isAsync && return_info.ownedBy) { - return_info.ownedByIndex = nameToArgIndex[return_info.ownedBy]; + return_info.ownedBy.forEach(function (argName) { + return_info.ownedByIndices.push(nameToArgIndex[return_info.ownedBy]); + }) } result.push(return_info); @@ -65,11 +69,21 @@ module.exports = function(fn, argReturnsOnly, isAsync) { return_info.__proto__ = fn.return; return_info.isAsync = isAsync; - return_info.hasOwner = !!(return_info.ownedByThis || return_info.ownerFn); - if (return_info.ownedByThis) { - return_info.ownedBy = thisArgName; + return_info.hasOwner = !return_info.selfOwned && + !!(return_info.ownedBy || return_info.ownedByThis || return_info.ownerFn); + return_info.ownedByIndices = []; + return_info.ownedBy = return_info.ownedBy || []; + + if (return_info.ownedByThis && isAsync) { + return_info.ownedBy.push(thisArgName); + } + + if (!isAsync) { + return_info.ownedByIndices = return_info.ownedBy.map(function (ownerName) { + return nameToArgIndex[ownerName]; + }); } - return_info.ownedByIndex = -1; + return_info.parsedName = return_info.name && isAsync ? "baton->" + return_info.name : "result"; return_info.isCppClassIntType = ~['Uint32', 'Int32'].indexOf(return_info.cppClassName); return_info.parsedClassName = (return_info.cppClassName || '').toLowerCase() + "_t"; diff --git a/generate/templates/partials/convert_to_v8.cc b/generate/templates/partials/convert_to_v8.cc index fd3de20c4..908f99018 100644 --- a/generate/templates/partials/convert_to_v8.cc +++ b/generate/templates/partials/convert_to_v8.cc @@ -2,11 +2,11 @@ {% if cppClassName == 'String' %} if ({{= parsedName =}}){ {% if size %} - to = Nan::New({{= parsedName =}}, {{ size }}).ToLocalChecked(); + to = Nan::New({{= parsedName =}}, {{ size }}).ToLocalChecked(); {% elsif cType == 'char **' %} - to = Nan::New(*{{= parsedName =}}).ToLocalChecked(); + to = Nan::New(*{{= parsedName =}}).ToLocalChecked(); {% else %} - to = Nan::New({{= parsedName =}}).ToLocalChecked(); + to = Nan::New({{= parsedName =}}).ToLocalChecked(); {% endif %} } else { @@ -35,7 +35,7 @@ {% if size %} v8::Local tmpArray = Nan::New({{= parsedName =}}->{{ size }}); for (unsigned int i = 0; i < {{= parsedName =}}->{{ size }}; i++) { - Nan::Set(tmpArray, Nan::New(i), Nan::New({{= parsedName =}}->{{ key }}[i]).ToLocalChecked()); + Nan::Set(tmpArray, Nan::New(i), Nan::New({{= parsedName =}}->{{ key }}[i]).ToLocalChecked()); } {% else %} v8::Local tmpArray = Nan::New({{= parsedName =}}); @@ -47,7 +47,7 @@ to = Nan::Null(); {% else %} if ({{= parsedName =}}) { - to = Nan::New({{= parsedName =}}->ptr, {{= parsedName = }}->size).ToLocalChecked(); + to = Nan::New({{= parsedName =}}->ptr, {{= parsedName = }}->size).ToLocalChecked(); } else { to = Nan::Null(); @@ -61,6 +61,34 @@ {% endif %} if ({{= parsedName =}} != NULL) { + {% if hasOwner %} + v8::Local owners = Nan::New(0); + {% if ownedBy %} + {% if isAsync %} + {% each ownedBy as owner %} + Nan::Set(owners, Nan::New(owners->Length()), this->GetFromPersistent("{{= owner =}}")->ToObject()); + {% endeach %} + {% else %} + {% each ownedByIndices as ownedByIndex %} + Nan::Set(owners, Nan::New(owners->Length()), info[{{= ownedByIndex =}}]->ToObject()); + {% endeach %} + {% endif %} + {% endif %} + {%if isAsync %} + {% elsif ownedByThis %} + Nan::Set(owners, owners->Length(), info.This()); + {% endif %} + {% if ownerFn | toBool %} + Nan::Set( + owners, + Nan::New(owners->Length()), + {{= ownerFn.singletonCppClassName =}}::New( + {{= ownerFn.name =}}({{= parsedName =}}), + true + )->ToObject() + ); + {% endif %} + {% endif %} // {{= cppClassName }} {{= parsedName }} {% if cppClassName == 'Wrapper' %} to = {{ cppClassName }}::New({{= parsedName =}}); @@ -69,19 +97,7 @@ {{= parsedName =}}, {{ selfFreeing|toBool }} {% if hasOwner %} - , - {% if ownerFn | toBool %} - {{= ownerFn.singletonCppClassName =}}::New( - {{= ownerFn.name =}}({{= parsedName =}}), - true - )->ToObject() - {% elsif isAsync %} - this->GetFromPersistent("{{= ownedBy =}}")->ToObject() - {% elsif ownedByThis %} - info.This() - {% else %} - info[{{= ownedByIndex =}}]->ToObject() - {% endif %} + , owners {% endif %} ); {% endif %} From 1e1b6394177671e6cfa643a558ba17235688a5c6 Mon Sep 17 00:00:00 2001 From: Tyler Wanek Date: Fri, 19 Oct 2018 09:29:53 -0700 Subject: [PATCH 10/13] Ownership updates to descriptor and supplement co-authored-by: Steve King We walked the entire file and compared documentation + struct implementation to try and glean ownership relationships between out parameters and arguments. Since we had to walk this whole file, and it took about 2 1/2 days, we also spent a little bit of time touching up groupings, definitions, and ignore settings. --- generate/input/descriptor.json | 969 ++++++++++++++++++------- generate/input/libgit2-supplement.json | 160 +++- 2 files changed, 854 insertions(+), 275 deletions(-) diff --git a/generate/input/descriptor.json b/generate/input/descriptor.json index 6b581c5fd..fe642875d 100644 --- a/generate/input/descriptor.json +++ b/generate/input/descriptor.json @@ -88,22 +88,22 @@ }, "git_annotated_commit_from_ref": { "return": { - "ownedBy": "repo" + "ownedBy": ["repo"] } }, "git_annotated_commit_from_fetchhead": { "return": { - "ownedBy": "repo" + "ownedBy": ["repo"] } }, "git_annotated_commit_lookup": { "return": { - "ownedBy": "repo" + "ownedBy": ["repo"] } }, "git_annotated_commit_from_revspec": { "return": { - "ownedBy": "repo" + "ownedBy": ["repo"] } } } @@ -145,14 +145,14 @@ "git_blame_buffer": { "args": { "out": { - "ownedBy": "reference" + "ownedBy": ["reference"] } } }, "git_blame_file": { "args": { "out": { - "ownedBy": "repo" + "ownedBy": ["repo"] }, "options": { "isOptional": true @@ -171,6 +171,9 @@ "return": { "ownedByThis": true } + }, + "git_blame_init_options": { + "ignore": true } } }, @@ -225,7 +228,10 @@ "isErrorCode": true } }, - "git_blob_create_fromchunks": { + "git_blob_create_fromstream": { + "ignore": true + }, + "git_blob_create_fromstream_commit": { "ignore": true }, "git_blob_dup": { @@ -283,9 +289,6 @@ }, "branch": { "functions": { - "git_branch_iterator_free": { - "ignore": true - }, "git_branch_create": { "args": { "force": { @@ -316,6 +319,12 @@ "isErrorCode": true } }, + "git_branch_iterator_free": { + "ignore": true + }, + "git_branch_iterator_new": { + "ignore": true + }, "git_branch_next": { "ignore": true }, @@ -466,6 +475,9 @@ "isErrorCode": true } }, + "git_checkout_init_options": { + "ignore": true + }, "git_checkout_tree": { "args": { "treeish": { @@ -489,6 +501,9 @@ "return": { "isErrorCode": true } + }, + "git_cherrypick_init_options": { + "ignore": true } } }, @@ -500,6 +515,9 @@ "isOptional": true } } + }, + "git_clone_init_options": { + "ignore": true } } }, @@ -692,6 +710,19 @@ "isErrorCode": true } }, + "git_config_find_programdata": { + "isAsync": true, + "return": { + "isErrorCode": true + }, + "args": { + "out": { + "isReturn": true, + "isSelf": false, + "shouldAlloc": true + } + } + }, "git_config_find_system": { "isAsync": true, "args": { @@ -732,7 +763,7 @@ "args": { "out": { "isReturn": true, - "ownedBy": "cfg" + "ownedByThis": true } } }, @@ -775,6 +806,9 @@ "isErrorCode": true } }, + "git_config_init_backend": { + "ignore": true + }, "git_config_iterator_free": { "ignore": true }, @@ -784,8 +818,14 @@ "git_config_iterator_new": { "ignore": true }, - "git_config_init_backend": { - "ignore": true + "git_config_lock": { + "isAsync": true, + "args": { + "tx": { + "isReturn": true, + "ownedByThis": true + } + } }, "git_config_lookup_map_value": { "ignore": true @@ -834,36 +874,32 @@ "git_config_parse_path": { "ignore": true }, - "git_config_refresh": { - "ignore": true - }, "git_config_set_bool": { - "ignore": true + "isAsync": true, + "return": { + "isErrorCode": true + } }, "git_config_set_int32": { - "ignore": true - }, - "git_config_set_string": { "isAsync": true, "return": { "isErrorCode": true } }, - "git_config_snapshot": { - "ignore": true + "git_config_set_int64": { + "isAsync": true, + "return": { + "isErrorCode": true + } }, - "git_config_find_programdata": { + "git_config_set_string": { "isAsync": true, "return": { "isErrorCode": true - }, - "args": { - "out": { - "isReturn": true, - "isSelf": false, - "shouldAlloc": true - } } + }, + "git_config_snapshot": { + "ignore": true } }, "dependencies": [ @@ -909,6 +945,9 @@ } } }, + "cred_default": { + "ignore": true + }, "cred_ssh_custom": { "ignore": true }, @@ -919,17 +958,10 @@ "ignore": true }, "cred_username": { - "fields": { - "username": { - "cppClassName": "String", - "cType": "char *" - } - } + "ignore": true }, "cred_userpass_payload": { - "cDependencies": [ - "git2/cred_helpers.h" - ] + "ignore": true }, "cred_userpass_plaintext": { "ignore": true @@ -937,6 +969,9 @@ "describe": { "ignore": true }, + "describe_format_options": { + "hasConstructor": true + }, "diff": { "selfFreeing": true, "cDependencies": [ @@ -1021,20 +1056,47 @@ "git_diff_free": { "ignore": true }, + "git_diff_get_perfdata": { + "isAsync": false, + "args": { + "out": { + "isReturn": true, + "shouldAlloc": true + }, + "diff": { + "isSelf": true + } + }, + "return": { + "isErrorCode": true + } + }, "git_diff_get_stats": { - "ignore": true + "isAsync": true, + "args": { + "out": { + "isReturn": true, + "ownedByThis": true + }, + "diff": { + "isSelf": true + } + }, + "return": { + "isErrorCode": true + } }, "git_diff_index_to_index": { "args": { "diff": { - "ownedBy": "repo" + "ownedBy": ["repo"] } } }, "git_diff_index_to_workdir": { "args": { "diff": { - "ownedBy": "repo" + "ownedBy": ["repo"] }, "index": { "isOptional": true @@ -1047,12 +1109,12 @@ "git_diff_init_options": { "ignore": true }, - "git_diff_is_sorted_icase": { - "ignore": true - }, "git_diff_merge": { "isAsync": true, "args": { + "from": { + "ownedByThis": true + }, "onto": { "isSelf": true } @@ -1064,6 +1126,9 @@ "git_diff_num_deltas_of_type": { "ignore": true }, + "git_diff_patchid_init_options": { + "ignore": true + }, "git_diff_print": { "ignore": true }, @@ -1073,21 +1138,6 @@ "git_diff_print_callback__to_file_handle": { "ignore": true }, - "git_diff_stats_deletions": { - "ignore": true - }, - "git_diff_stats_files_changed": { - "ignore": true - }, - "git_diff_stats_free": { - "ignore": true - }, - "git_diff_stats_insertions": { - "ignore": true - }, - "git_diff_stats_to_buf": { - "ignore": true - }, "git_diff_status_char": { "ignore": true }, @@ -1107,7 +1157,7 @@ "git_diff_tree_to_index": { "args": { "diff": { - "ownedBy": "repo" + "ownedBy": ["repo"] }, "old_tree": { "isOptional": true @@ -1123,7 +1173,7 @@ "git_diff_tree_to_tree": { "args": { "diff": { - "ownedBy": "repo" + "ownedBy": ["repo"] }, "old_tree": { "isOptional": true @@ -1139,7 +1189,7 @@ "git_diff_tree_to_workdir": { "args": { "diff": { - "ownedBy": "repo" + "ownedBy": ["repo"] }, "old_tree": { "isOptional": true @@ -1152,7 +1202,7 @@ "git_diff_tree_to_workdir_with_index": { "args": { "diff": { - "ownedBy": "repo" + "ownedBy": ["repo"] }, "old_tree": { "isOptional": true @@ -1176,6 +1226,7 @@ "ignore": true }, "diff_perfdata": { + "selfFreeing": true, "cDependencies": [ "git2/sys/diff.h" ] @@ -1184,7 +1235,42 @@ "ignore": true }, "diff_stats": { - "selfFreeing": true + "selfFreeing": true, + "functions": { + "git_diff_stats_deletions": { + "args": { + "stats": { + "isSelf": true + } + } + }, + "git_diff_stats_files_changed": { + "args": { + "stats": { + "isSelf": true + } + } + }, + "git_diff_stats_free": { + "ignore": true + }, + "git_diff_stats_insertions": { + "args": { + "stats": { + "isSelf": true + } + } + }, + "git_diff_stats_to_buf": { + "cppFunctionName": "DiffStatsToBuf", + "isAsync": true, + "args": { + "stats": { + "isSelf": true + } + } + } + } }, "error_code": { "values": { @@ -1193,119 +1279,79 @@ } } }, + "fetch": { + "functions": { + "git_fetch_init_options": { + "ignore": true + } + } + }, "filter": { "selfFreeing": false, "hasConstructor": true, + "cDependencies": [ + "git2/sys/filter.h" + ], + "fields": { + "stream": { + "ignore": true + } + } + }, + "filter_source": { "functions": { - "git_filter_list_apply_to_blob": { - "async": true, - "return": { - "isErrorCode": true - }, + "git_filter_source_filemode": { + "isPrototypeMethod": true, "args": { - "out": { - "isReturn": true, - "shouldAlloc": true - }, - "filters": { + "src": { "isSelf": true } } }, - "git_filter_list_apply_to_data": { - "async": true, - "return": { - "isErrorCode": true - }, + "git_filter_source_flags": { + "isPrototypeMethod": true, "args": { - "out": { - "isReturn": true, - "shouldAlloc": true - }, - "filters": { + "src": { "isSelf": true } } }, - "git_filter_list_apply_to_file": { - "async": true, - "return": { - "isErrorCode": true - }, + "git_filter_source_id": { + "isPrototypeMethod": true, "args": { - "out": { - "isReturn": true, - "shouldAlloc": true - }, - "filters": { + "src": { "isSelf": true } + }, + "return": { + "ownedByThis": true } }, - "git_filter_list_free": { - "async": true, + "git_filter_source_mode": { + "isPrototypeMethod": true, "args": { - "filters": { + "src": { "isSelf": true } } }, - "git_filter_list_load": { - "async": true, - "return": { - "isErrorCode": true - }, + "git_filter_source_path": { + "isPrototypeMethod": true, "args": { - "filters": { - "isReturn": true, - "cType": "git_filter_list **", - "ownedBy": "repo" - }, - "repo": { - "cType": "git_repository *" - }, - "blob": { - "isOptional": true, - "cType": "git_blob *" + "src": { + "isSelf": true } } }, - "git_filter_list_push": { - "ignore": true - }, - "git_filter_source_filemode": { - "ignore": true - }, - "git_filter_source_flags": { - "ignore": true - }, - "git_filter_source_id": { - "ignore": true - }, - "git_filter_source_mode": { - "ignore": true - }, - "git_filter_source_options": { - "ignore": true - }, - "git_filter_source_path": { - "ignore": true - }, "git_filter_source_repo": { - "ignore": true + "isPrototypeMethod": true, + "args": { + "src": { + "isSelf": true + } + } } }, - "cDependencies": [ - "git2/sys/filter.h" - ], - "fields": { - "stream": { - "ignore": true - } - } - }, - "filter_source": { - "ignore": false, "cDependencies": [ "git2/sys/filter.h" ] @@ -1313,8 +1359,47 @@ "filter_list": { "selfFreeing": true, "functions": { + "git_filter_list_apply_to_blob": { + "isPrototypeMethod": true, + "args": { + "filters": { + "isSelf": true + }, + "out": { + "shouldAlloc": true + } + } + }, + "git_filter_list_apply_to_data": { + "isPrototypeMethod": true, + "args": { + "filters": { + "isSelf": true + }, + "out": { + "shouldAlloc": true + } + } + }, + "git_filter_list_apply_to_file": { + "isPrototypeMethod": true, + "args": { + "filters": { + "isSelf": true + }, + "out": { + "shouldAlloc": true + } + } + }, "git_filter_list_free": { "ignore": true + }, + "git_filter_list_new": { + "ignore": true + }, + "git_filter_list_push": { + "ignore": true } }, "dependencies": [ @@ -1454,15 +1539,15 @@ "args": { "ancestor_out": { "isReturn": true, - "ownedBy": "index" + "ownedBy": ["index"] }, "our_out": { "isReturn": true, - "ownedBy": "index" + "ownedBy": ["index"] }, "their_out": { "isReturn": true, - "ownedBy": "index" + "ownedBy": ["index"] } }, "isAsync": true, @@ -1528,9 +1613,13 @@ "git_index_open": { "isAsync": true, "return": { + "selfOwned": true, "isErrorCode": true } }, + "git_index_owner": { + "ignore": true + }, "git_index_read": { "args": { "force": { @@ -1583,6 +1672,12 @@ "isErrorCode": true } }, + "git_index_reuc_get_byindex": { + "ignore": true + }, + "git_index_reuc_get_bypath": { + "ignore": true + }, "git_index_update_all": { "args": { "pathspec": { @@ -1633,37 +1728,10 @@ "ignoreInit": true }, "indexer": { - "selfFreeing": true, - "cType": "git_indexer", - "functions": { - "git_indexer_append": { - "ignore": true - }, - "git_indexer_free": { - "ignore": true - }, - "git_indexer_hash": { - "return": { - "ownedByThis": true - } - }, - "git_indexer_new": { - "ignore": true - } - } + "ignore": true }, "mempack": { - "functions": { - "git_mempack_dump": { - "ignore": true - }, - "git_mempack_new": { - "ignore": true - }, - "git_mempack_reset": { - "ignore": true - } - } + "ignore": true }, "merge": { "functions": { @@ -1718,16 +1786,6 @@ } } }, - "git_merge_trees": { - "args": { - "ancestor_tree": { - "isOptional": true - }, - "opts": { - "isOptional": true - } - } - }, "git_merge_file": { "ignore": true }, @@ -1737,29 +1795,74 @@ "git_merge_file_init_options": { "ignore": true }, - "git_merge_file_result_free": { + "git_merge_init_options": { "ignore": true + }, + "git_merge_trees": { + "args": { + "ancestor_tree": { + "isOptional": true + }, + "opts": { + "isOptional": true + } + } } } }, "merge_driver": { "ignore": true }, + "merge_driver_source": { + "ignore": true + }, + "merge_file_result": { + "ignore": true + }, + "merge_result": { + "ignore": true + }, "message": { - "functions": { - "git_message_prettify": { - "ignore": true - }, - "git_message_trailers": { - "ignore": true - } - } + "ignore": true + }, + "message_trailer": { + "ignore": true + }, + "message_trailer_array": { + "ignore": true }, "note": { "selfFreeing": true, "functions": { - "git_note_iterator_free": { - "ignore": true + "git_note_author": { + "return": { + "ownedByThis": true + } + }, + "git_note_commit_create": { + "args": { + "notes_commit_out": { + "isReturn": true, + "shouldAlloc": true + }, + "notes_blob_out": { + "isReturn": true, + "shouldAlloc": true + } + } + }, + "git_note_commit_remove": { + "isAsync": true, + "args": { + "notes_commit_out": { + "isReturn": true + } + } + }, + "git_note_comitter": { + "return": { + "ownedByThis": true + } }, "git_note_create": { "args": { @@ -1768,6 +1871,12 @@ } } }, + "git_note_foreach": { + "isAsync": true, + "return": { + "isErrorCode": true + } + }, "git_note_free": { "ignore": true }, @@ -1776,13 +1885,16 @@ "ownedByThis": true } }, - "git_note_remove": { - "isAsync": true, - "return": { - "isErrorCode": true - } + "git_note_iterator_free": { + "ignore": true }, - "git_note_foreach": { + "git_note_iterator_new": { + "ignore": true + }, + "git_note_next": { + "ignore": true + }, + "git_note_remove": { "isAsync": true, "return": { "isErrorCode": true @@ -1826,6 +1938,9 @@ "git_odb_add_backend": { "ignore": true }, + "git_odb_add_disk_alternate": { + "ignore": true + }, "git_odb_backend_loose": { "ignore": true }, @@ -1836,9 +1951,29 @@ "ignore": true }, "git_odb_exists": { - "ignore": true + "ignore": true, + "isAsync": true, + "args": { + "db": { + "isSelf": true + } + } }, "git_odb_exists_prefix": { + "isAsync": true, + "args": { + "db": { + "isSelf": true + }, + "out": { + "isReturn": true + } + }, + "return": { + "isErrorCode": true + } + }, + "git_odb_expand_ids": { "ignore": true }, "git_odb_foreach": { @@ -1854,7 +1989,15 @@ "ignore": true }, "git_odb_hashfile": { - "ignore": true + "isAsync": true, + "args": { + "out": { + "isReturn": true + } + }, + "return": { + "isErrorCode": true + } }, "git_odb_init_backend": { "ignore": true @@ -1871,27 +2014,27 @@ "git_odb_open_wstream": { "ignore": true }, + "git_odb_read": { + "cppFunctionName": "OdbRead", + "args": { + "out": { + "ownedByThis": true + } + } + }, "git_odb_read_header": { "ignore": true }, "git_odb_read_prefix": { - "ignore": true + "args": { + "out": { + "ownedByThis": true + } + } }, "git_odb_refresh": { "ignore": true }, - "git_odb_stream_finalize_write": { - "ignore": true - }, - "git_odb_stream_free": { - "ignore": true - }, - "git_odb_stream_read": { - "ignore": true - }, - "git_odb_stream_write": { - "ignore": true - }, "git_odb_write": { "args": { "data": { @@ -1916,7 +2059,11 @@ }, "ignore": true }, + "odb_expand_id": { + "ignore": true + }, "odb_object": { + "selfFreeing": true, "functions": { "git_odb_object_data": { "return": { @@ -1924,6 +2071,9 @@ "jsClassName": "Buffer" } }, + "git_odb_object_dup": { + "ignore": true + }, "git_odb_object_free": { "ignore": true }, @@ -1978,15 +2128,6 @@ "git_oid_pathfmt": { "ignore": true }, - "git_oid_shorten_add": { - "ignore": true - }, - "git_oid_shorten_free": { - "ignore": true - }, - "git_oid_shorten_new": { - "ignore": true - }, "git_oid_tostr": { "ignore": true } @@ -1998,7 +2139,22 @@ } }, "oid_shorten": { - "selfFreeing": true + "selfFreeing": true, + "functions": { + "git_oid_shorten_new": { + "isPrototypeMethod": false + }, + "git_oid_shorten_add": { + "args": { + "os": { + "isSelf": true + } + } + }, + "git_oid_shorten_free": { + "ignore": true + } + } }, "oidarray": { "selfFreeing": true, @@ -2022,9 +2178,33 @@ "git_packbuilder_free": { "ignore": true }, - "git_packbuilder_hash": { + "git_packbuilder_hash": { + "return": { + "ownedByThis": true + } + }, + "git_packbuilder_insert": { + "isAsync": true, + "return": { + "isErrorCode": true + } + }, + "git_packbuilder_insert_commit": { + "isAsync": true, + "return": { + "isErrorCode": true + } + }, + "git_packbuilder_insert_recur": { + "isAsync": true, + "return": { + "isErrorCode": true + } + }, + "git_packbuilder_insert_walk": { + "isAsync": true, "return": { - "ownedByThis": true + "isErrorCode": true } }, "git_packbuilder_new": { @@ -2035,6 +2215,9 @@ }, "git_packbuilder_write": { "ignore": true + }, + "git_packbuilder_write_buf": { + "ignore": true } } }, @@ -2056,13 +2239,19 @@ "git_patch_from_diff": { "isAsync": true, "return": { - "isErrorCode": true + "ownedBy": ["diff"] + } + }, + "git_patch_get_delta": { + "return": { + "ownedByThis": true } }, "git_patch_get_hunk": { "args": { "out": { - "returnName": "hunk" + "returnName": "hunk", + "ownedByThis": true }, "lines_in_hunk": { "shouldAlloc": true, @@ -2077,6 +2266,11 @@ }, "git_patch_get_line_in_hunk": { "isAsync": true, + "args": { + "out": { + "ownedByThis": true + } + }, "return": { "isErrorCode": true } @@ -2114,11 +2308,64 @@ "git_pathspec_free": { "ignore": true }, - "git_pathspec_match_list_free": { - "ignore": true + "git_pathspec_match_diff": { + "isAsync": true, + "args": { + "out": { + "ownedBy": ["diff"], + "ownedByThis": true + } + } + }, + "git_pathspec_match_index": { + "isAsync": true, + "args": { + "out": { + "ownedBy": ["index"], + "ownedByThis": true + } + } }, "git_pathspec_new": { "isAsync": false + }, + "git_pathspec_match_tree": { + "isAsync": true, + "args": { + "out": { + "ownedBy": ["tree"], + "ownedByThis": true + } + } + }, + "git_pathspec_match_workdir": { + "isAsync": true, + "args": { + "out": { + "ownedBy": ["repo"], + "ownedByThis": true + } + } + } + } + }, + "pathspec_match_list": { + "selfFreeing": true, + "functions": { + "git_pathspec_match_list_diff_entry": { + "return": { + "ownedByThis": true + } + }, + "git_pathspec_match_list_free": { + "ignore": true + } + } + }, + "proxy": { + "functions": { + "git_proxy_init_options": { + "ignore": true } } }, @@ -2128,6 +2375,22 @@ "rebase": { "selfFreeing": true, "functions": { + "git_rebase_abort": { + "isAsync": true, + "args": { + "rebase": { + "cType": "git_rebase *", + "cppClassName": "GitRebase", + "jsClassName": "Rebase", + "isOptional": false, + "isSelf": true, + "isReturn": false + } + }, + "return": { + "isErrorCode": true + } + }, "git_rebase_commit": { "isAsync": true, "args": { @@ -2164,6 +2427,9 @@ }, "git_rebase_init": { "args": { + "out": { + "ownedBy": ["repo"] + }, "upstream": { "isOptional": true }, @@ -2178,20 +2444,40 @@ } } }, - "git_rebase_abort": { - "isAsync": true, + "git_rebase_init_options": { + "ignore": true + }, + "git_rebase_inmemory_index": { "args": { - "rebase": { - "cType": "git_rebase *", - "cppClassName": "GitRebase", - "jsClassName": "Rebase", - "isOptional": false, + "out": { + "ownedByThis": true + } + } + }, + "git_rebase_next": { + "args": { + "operation": { + "isReturn": true, + "ownedByThis": true + } + } + }, + "git_rebase_open": { + "args": { + "out": { "isSelf": true, - "isReturn": false + "ownedBy": ["repo"] } - }, + } + }, + "git_rebase_operation_byindex": { "return": { - "isErrorCode": true + "ownedByThis": true + } + }, + "git_rebase_operation_current": { + "return": { + "ownedByThis": true } } } @@ -2213,6 +2499,13 @@ }, "git_refdb_new": { "ignore": true + }, + "git_refdb_open": { + "args": { + "out": { + "ownedBy": ["repo"] + } + } } } }, @@ -2233,6 +2526,9 @@ "git_reference__alloc_symbolic": { "ignore": true }, + "git_reference_dup": { + "ignore": true + }, "git_reference_foreach": { "ignore": true }, @@ -2313,6 +2609,13 @@ "git_reflog_free": { "ignore": true }, + "git_reflog_read": { + "args": { + "out": { + "ownedBy": ["repo"] + } + } + }, "git_reflog_write": { "isAsync": true, "isSelf": true, @@ -2324,6 +2627,11 @@ }, "reflog_entry": { "functions": { + "git_reflog_entry_committer": { + "return": { + "ownedByThis": true + } + }, "git_reflog_entry_id_new": { "return": { "ownedByThis": true @@ -2337,12 +2645,24 @@ } }, "refspec": { + "selfFreeing": true, "cType": "git_refspec", "functions": { - "git_refspec_rtransform": { + "git_refspec_free": { "ignore": true }, - "git_refspec_string": { + "git_refspec_parse": { + "isAsync": true, + "args": { + "refspec": { + "isReturn": true + } + }, + "return": { + "isErrorCode": true + } + }, + "git_refspec_rtransform": { "ignore": true }, "git_refspec_transform": { @@ -2368,6 +2688,14 @@ "isErrorCode": true } }, + "git_remote_create_detached": { + "isAsync": true, + "args": { + "out": { + "selfOwned": true + } + } + }, "git_remote_connect": { "isAsync": true, "return": { @@ -2410,6 +2738,9 @@ "isErrorCode": true } }, + "git_remote_dup": { + "ignore": true + }, "git_remote_fetch": { "args": { "reflog_message": { @@ -2464,6 +2795,9 @@ "ownedByThis": true } }, + "git_remote_init_callbacks": { + "ignore": true + }, "git_remote_list": { "args": { "out": { @@ -2565,9 +2899,6 @@ } } }, - "git_repository_init_init_options": { - "ignore": true - }, "git_repository_fetchhead_foreach": { "isAsync": true, "return": { @@ -2583,6 +2914,9 @@ "git_repository_ident": { "ignore": true }, + "git_repository_init_init_options": { + "ignore": true + }, "git_repository_mergehead_foreach": { "isAsync": true, "return": { @@ -2595,6 +2929,34 @@ "git_repository_new": { "ignore": true }, + "git_repository_odb": { + "isAsync": true, + "args": { + "odb": { + "ownedByThis": true + }, + "repo": { + "isSelf": true + } + }, + "return": { + "isErrorCode": true + } + }, + "git_repository_refdb": { + "isAsync": true, + "args": { + "out": { + "ownedByThis": true + }, + "repo": { + "isSelf": true + } + }, + "return": { + "isErrorCode": true + } + }, "git_repository_reinit_filesystem": { "ignore": true }, @@ -2736,6 +3098,9 @@ "git_signature_default": { "isAsync": false }, + "git_signature_dup": { + "ignore": true + }, "git_signature_free": { "ignore": true }, @@ -2783,6 +3148,9 @@ "isErrorCode": true } }, + "git_stash_apply_init_options": { + "ignore": true + }, "git_stash_pop": { "isAsync": true, "return": { @@ -2808,7 +3176,10 @@ ], "functions": { "git_status_byindex": { - "isAsync": false + "isAsync": false, + "return": { + "ownedBy": ["statuslist"] + } }, "git_status_file": { "isAsync": true, @@ -2848,6 +3219,9 @@ "git_status_list_new": { "isAsync": true, "args": { + "out": { + "ownedBy": ["repo"] + }, "opts": { "isOptional": true } @@ -2859,6 +3233,7 @@ } }, "strarray": { + "selfFreeing": true, "functions": { "git_strarray_free": { "ignore": true @@ -3019,6 +3394,9 @@ "return": { "isErrorCode": true } + }, + "git_submodule_update_init_options": { + "ignore": true } } }, @@ -3068,6 +3446,9 @@ }, "isAsync": true }, + "git_tag_dup": { + "ignore": true + }, "git_tag_foreach": { "ignore": true }, @@ -3174,6 +3555,21 @@ } } }, + "transaction": { + "selfFreeing": true, + "functions": { + "git_transaction_free": { + "ignore": true + }, + "git_transaction_new": { + "args": { + "out": { + "ownedBy": ["repo"] + } + } + } + } + }, "transfer_progress": { "dupFunction": "git_transfer_progress_dup" }, @@ -3184,6 +3580,9 @@ "git_transport_dummy": { "ignore": true }, + "git_transport_init": { + "ignore": true + }, "git_transport_local": { "ignore": true }, @@ -3195,6 +3594,12 @@ }, "git_transport_smart": { "ignore": true + }, + "git_transport_ssh_with_paths": { + "ignore": true + }, + "git_transport_unregister": { + "ignore": true } }, "cDependencies": [ @@ -3208,24 +3613,38 @@ "singletonCppClassName": "GitRepository" }, "functions": { + "git_tree_dup": { + "ignore": true + }, "git_tree_entry_byid": { "return": { - "ownedByThis": true, - "selfFreeing": false + "ownedByThis": true } }, "git_tree_entry_byindex": { "jsFunctionName": "_entryByIndex", "return": { - "ownedByThis": true, - "selfFreeing": false + "ownedByThis": true } }, "git_tree_entry_byname": { "jsFunctionName": "_entryByName", "return": { - "ownedByThis": true, - "selfFreeing": false + "ownedByThis": true + } + }, + "git_tree_entry_bypath": { + "isAsync": true, + "args": { + "out": { + "ownedByThis": true + }, + "root": { + "isSelf": true + } + }, + "return": { + "isErrorCode": true } }, "git_tree_entrycount": { @@ -3255,7 +3674,6 @@ }, "git_treebuilder_get": { "return": { - "selfFreeing": false, "ownedByThis": true } }, @@ -3284,6 +3702,9 @@ }, "git_treebuilder_new": { "args": { + "out": { + "ownedBy": ["repo"] + }, "source": { "isOptional": true } @@ -3296,6 +3717,9 @@ "dupFunction": "git_tree_entry_dup", "freeFunctionName": "git_tree_entry_free", "functions": { + "git_tree_entry_dup": { + "ignore": true + }, "git_tree_entry_free": { "ignore": true }, @@ -3322,12 +3746,33 @@ "cType": "git_worktree", "freeFunctionName": "git_worktree_free", "functions": { - "git_worktree_free": { - "ignore": true + "git_worktree_add": { + "args": { + "out": { + "ownedBy": ["repo"] + } + } }, "git_worktree_add_init_options": { "ignore": true }, + "git_worktree_free": { + "ignore": true + }, + "git_worktree_lookup": { + "args": { + "out": { + "ownedBy": ["repo"] + } + } + }, + "git_worktree_open_from_repository": { + "args": { + "out": { + "ownedBy": ["repo"] + } + } + }, "git_worktree_prune_init_options": { "ignore": true } diff --git a/generate/input/libgit2-supplement.json b/generate/input/libgit2-supplement.json index 0c6e22a50..3697a3485 100644 --- a/generate/input/libgit2-supplement.json +++ b/generate/input/libgit2-supplement.json @@ -345,6 +345,16 @@ "git_annotated_commit_lookup" ] ], + [ + "diff_stats", + [ + "git_diff_stats_files_changed", + "git_diff_stats_insertions", + "git_diff_stats_deletions", + "git_diff_stats_to_buf", + "git_diff_stats_free" + ] + ], [ "filter_list", [ @@ -366,6 +376,12 @@ "git_filter_source_flags" ] ], + [ + "merge_file_result", + [ + "git_merge_file_result_free" + ] + ], [ "odb_object", [ @@ -377,12 +393,40 @@ "git_odb_object_type" ] ], + [ + "odb_stream", + [ + "git_odb_stream_finalize_write", + "git_odb_stream_free", + "git_odb_stream_read", + "git_odb_stream_write" + ] + ], + [ + "oid_shorten", + [ + "git_oid_shorten_add", + "git_oid_shorten_free", + "git_oid_shorten_new" + ] + ], [ "patch", [ "git_patch_convenient_from_diff" ] ], + [ + "pathspec_match_list", + [ + "git_pathspec_match_list_diff_entry", + "git_pathspec_match_list_entry", + "git_pathspec_match_list_entrycount", + "git_pathspec_match_list_failed_entry", + "git_pathspec_match_list_failed_entrycount", + "git_pathspec_match_list_free" + ] + ], [ "reflog_entry", [ @@ -500,6 +544,72 @@ ] } ], + [ + "git_describe_format_options", + { + "type": "struct", + "fields": [ + { + "type": "unsigned int", + "name": "version" + }, + { + "type": "unsigned int", + "name": "abbreviated_size" + }, + { + "type": "int", + "name": "always_use_long_format" + }, + { + "type": "const char *", + "name": "dirty_suffix" + } + ], + "used": { + "needs": [ + "git_describe_init_format_options" + ] + } + } + ], + [ + "git_describe_options", + { + "type": "struct", + "fields": [ + { + "type": "unsigned int", + "name": "version" + }, + { + "type": "unsigned int", + "name": "max_candidates_tags" + }, + { + "type": "unsigned int", + "name": "describe_strategy" + }, + { + "type": "const char *", + "name": "pattern" + }, + { + "type": "int", + "name": "only_follow_first_parent" + }, + { + "type": "int", + "name": "show_commit_oid_as_fallback" + } + ], + "used": { + "needs": [ + "git_describe_init_options" + ] + } + } + ], [ "git_filter", { @@ -916,23 +1026,25 @@ "git_annotated_commit_lookup" ] }, - "filter": { + "diff": { "functions": [ - "git_filter_list_new", - "git_filter_list_push", - "git_filter_list_stream_blob", - "git_filter_list_stream_data", - "git_filter_list_stream_file", - "git_filter_source_filemode", - "git_filter_source_flags", - "git_filter_source_id", - "git_filter_source_mode", - "git_filter_source_path", - "git_filter_source_repo" + "git_diff_stats_files_changed", + "git_diff_stats_insertions", + "git_diff_stats_deletions", + "git_diff_stats_to_buf", + "git_diff_stats_free" ] }, "merge": { "functions": [ + "git_merge_driver_lookup", + "git_merge_driver_register", + "git_merge_driver_source_ancestor", + "git_merge_driver_source_ours", + "git_merge_driver_source_repo", + "git_merge_driver_source_theirs", + "git_merge_driver_unregister", + "git_merge_file_result_free", "git_merge_head_free", "git_merge_head_from_fetchhead", "git_merge_head_from_id", @@ -952,7 +1064,28 @@ "git_odb_object_free", "git_odb_object_id", "git_odb_object_size", - "git_odb_object_type" + "git_odb_object_type", + "git_odb_stream_finalize_write", + "git_odb_stream_free", + "git_odb_stream_read", + "git_odb_stream_write" + ] + }, + "oid": { + "functions": [ + "git_oid_shorten_add", + "git_oid_shorten_free", + "git_oid_shorten_new" + ] + }, + "pathspec": { + "functions": [ + "git_pathspec_match_list_diff_entry", + "git_pathspec_match_list_entry", + "git_pathspec_match_list_entrycount", + "git_pathspec_match_list_failed_entry", + "git_pathspec_match_list_failed_entrycount", + "git_pathspec_match_list_free" ] }, "reflog": { @@ -973,6 +1106,7 @@ }, "tree": { "functions": [ + "git_tree_entry_dup", "git_tree_entry_filemode", "git_tree_entry_filemode_raw", "git_tree_entry_free", From 1f8fd84bfaafb92acd470f309e8a1d48012b0eb9 Mon Sep 17 00:00:00 2001 From: Tyler Wanek Date: Fri, 19 Oct 2018 13:51:16 -0700 Subject: [PATCH 11/13] assign owner to wrappers --- generate/input/descriptor.json | 2 ++ 1 file changed, 2 insertions(+) diff --git a/generate/input/descriptor.json b/generate/input/descriptor.json index fe642875d..24ec653d3 100644 --- a/generate/input/descriptor.json +++ b/generate/input/descriptor.json @@ -277,6 +277,7 @@ }, "git_blob_rawcontent": { "return": { + "ownedByThis": true, "cppClassName": "Wrapper", "jsClassName": "Buffer" } @@ -2067,6 +2068,7 @@ "functions": { "git_odb_object_data": { "return": { + "ownedByThis": true, "cppClassName": "Wrapper", "jsClassName": "Buffer" } From ec91f71b396d5918b72486dd27e45ea7ec228e2f Mon Sep 17 00:00:00 2001 From: Tyler Wanek Date: Fri, 19 Oct 2018 13:53:10 -0700 Subject: [PATCH 12/13] Enable dup for any class with an ownerFn --- generate/input/descriptor.json | 21 --------------------- 1 file changed, 21 deletions(-) diff --git a/generate/input/descriptor.json b/generate/input/descriptor.json index 24ec653d3..096d4df9b 100644 --- a/generate/input/descriptor.json +++ b/generate/input/descriptor.json @@ -234,9 +234,6 @@ "git_blob_create_fromstream_commit": { "ignore": true }, - "git_blob_dup": { - "ignore": true - }, "git_blob_filtered_content": { "isAsync": true, "isPrototypeMethod": false, @@ -639,9 +636,6 @@ "git_commit_create_from_v": { "ignore": true }, - "git_commit_dup": { - "ignore": true - }, "git_commit_extract_signature": { "isAsync": true }, @@ -1910,9 +1904,6 @@ "singletonCppClassName": "GitRepository" }, "functions": { - "git_object_dup": { - "ignore": true - }, "git_object_free": { "ignore": true }, @@ -2528,9 +2519,6 @@ "git_reference__alloc_symbolic": { "ignore": true }, - "git_reference_dup": { - "ignore": true - }, "git_reference_foreach": { "ignore": true }, @@ -2740,9 +2728,6 @@ "isErrorCode": true } }, - "git_remote_dup": { - "ignore": true - }, "git_remote_fetch": { "args": { "reflog_message": { @@ -3448,9 +3433,6 @@ }, "isAsync": true }, - "git_tag_dup": { - "ignore": true - }, "git_tag_foreach": { "ignore": true }, @@ -3615,9 +3597,6 @@ "singletonCppClassName": "GitRepository" }, "functions": { - "git_tree_dup": { - "ignore": true - }, "git_tree_entry_byid": { "return": { "ownedByThis": true From a838ce8d88a82329ef3376149921e025d36d16d8 Mon Sep 17 00:00:00 2001 From: Tyler Wanek Date: Wed, 24 Oct 2018 17:38:05 -0700 Subject: [PATCH 13/13] Extraneous test --- test/tests/repository.js | 5 ----- 1 file changed, 5 deletions(-) diff --git a/test/tests/repository.js b/test/tests/repository.js index fbe791ee7..0474568a4 100644 --- a/test/tests/repository.js +++ b/test/tests/repository.js @@ -326,9 +326,4 @@ describe("Repository", function() { assert.equal(numMergeHeads, 1); }); }); - - it("can GC", function() { - global.gc(); - assert.equal(true, true); - }); });