fix: pr comments

Author: gmelodie

examples/echo/c_abi_bindings/echo.h

@@ -6,11 +6,9 @@
 #include <stdlib.h>
 #include <string.h>
 
-#ifndef NIMFFI_RET_OK
 #define NIMFFI_RET_OK 0
 #define NIMFFI_RET_ERR 1
 #define NIMFFI_RET_MISSING_CALLBACK 2
-#endif
 
 /* Flat wire structs — the C ABI. Strings are borrowed, NUL-terminated
    `const char*` valid only for the duration of the call they cross. */
@@ -60,6 +58,7 @@ typedef void (*EchoCreateFn)(int err_code, EchoCtx* ctx, const char* err_msg, vo
 typedef struct { EchoCreateFn fn; void* user_data; } EchoCreateBox;
 static void echo_create_trampoline(int ret, const char* ctx_addr, const char* err_msg, void* ud) {
     EchoCreateBox* box = (EchoCreateBox*)ud;
+    if (!box) return;
     if (!box->fn) { free(box); return; }
     if (ret != 0) {
         box->fn(ret, NULL, err_msg ? err_msg : "FFI create failed", box->user_data);

ffi/codegen/c_abi.nim

@@ -143,6 +143,28 @@ func paramByValue(nimType: string, ridesAsPtr: bool): bool =
     return true
   leafCTypeAbi(nimType.strip()).ok
 
+proc reqParamsAndAssigns(
+    reg: var AbiReg, extraParams: seq[FFIParamMeta]
+): tuple[params, assigns: seq[string]] =
+  ## The C parameter list + `ffi_req` field assignments shared by the ctor and
+  ## method wrappers: by-value params copy straight into the request struct,
+  ## by-const-pointer aggregates are dereferenced in.
+  var params, assigns: seq[string] = @[]
+  for ep in extraParams:
+    let rides = ep.ridesAsPtr()
+    let cType =
+      if rides:
+        CPtrType
+      else:
+        wireValueCType(reg, ep.typeName)
+    if paramByValue(ep.typeName, rides):
+      params.add(cType & " " & ep.name)
+      assigns.add("    ffi_req." & ep.name & " = " & ep.name & ";")
+    else:
+      params.add("const " & cType & "* " & ep.name)
+      assigns.add("    ffi_req." & ep.name & " = *" & ep.name & ";")
+  (params, assigns)
+
 proc methodReplyInfo(
     reg: var AbiReg, libType: string, m: FFIProcMeta
 ): tuple[fnType, replyParam: string] =
@@ -248,6 +270,7 @@ proc emitCtxAndCtor(
       "(int ret, const char* ctx_addr, const char* err_msg, void* ud) {"
   )
   lines.add("    " & createBox & "* box = (" & createBox & "*)ud;")
+  lines.add("    if (!box) return;")
   lines.add("    if (!box->fn) { free(box); return; }")
   lines.add("    if (ret != 0) {")
   lines.add(
@@ -281,21 +304,7 @@ proc emitCtxAndCtor(
   lines.add("")
   for ctor in ctors:
     let reqStruct = reqStructName(ctor)
-    var params: seq[string] = @[]
-    var assigns: seq[string] = @[]
-    for ep in ctor.extraParams:
-      let rides = ep.ridesAsPtr()
-      let cType =
-        if rides:
-          CPtrType
-        else:
-          wireValueCType(reg, ep.typeName)
-      if paramByValue(ep.typeName, rides):
-        params.add(cType & " " & ep.name)
-        assigns.add("    ffi_req." & ep.name & " = " & ep.name & ";")
-      else:
-        params.add("const " & cType & "* " & ep.name)
-        assigns.add("    ffi_req." & ep.name & " = *" & ep.name & ";")
+    let (params, assigns) = reqParamsAndAssigns(reg, ctor.extraParams)
     let head = "static inline int " & libName & "_ctx_create("
     let sig =
       if params.len > 0:
@@ -342,21 +351,7 @@ proc emitMethod(
   let stripped = stripLibPrefix(m.procName, m.libName)
   let reqStruct = reqStructName(m)
   let info = methodReplyInfo(reg, libType, m)
-  var params: seq[string] = @[]
-  var assigns: seq[string] = @[]
-  for ep in m.extraParams:
-    let rides = ep.ridesAsPtr()
-    let cType =
-      if rides:
-        CPtrType
-      else:
-        wireValueCType(reg, ep.typeName)
-    if paramByValue(ep.typeName, rides):
-      params.add(cType & " " & ep.name)
-      assigns.add("    ffi_req." & ep.name & " = " & ep.name & ";")
-    else:
-      params.add("const " & cType & "* " & ep.name)
-      assigns.add("    ffi_req." & ep.name & " = *" & ep.name & ";")
+  let (params, assigns) = reqParamsAndAssigns(reg, m.extraParams)
   let head =
     "static inline int " & libName & "_ctx_" & stripped & "(const " & ctxType & "* ctx, "
   let sig =
@@ -404,11 +399,9 @@ proc generateCAbiLibHeader*(
   lines.add("#include <stdlib.h>")
   lines.add("#include <string.h>")
   lines.add("")
-  lines.add("#ifndef NIMFFI_RET_OK")
   lines.add("#define NIMFFI_RET_OK 0")
   lines.add("#define NIMFFI_RET_ERR 1")
   lines.add("#define NIMFFI_RET_MISSING_CALLBACK 2")
-  lines.add("#endif")
   lines.add("")
   lines.add("/* Flat wire structs — the C ABI. Strings are borrowed, NUL-terminated")
   lines.add("   `const char*` valid only for the duration of the call they cross. */")

ffi/internal/c_macro_helpers.nim

@@ -632,9 +632,6 @@ proc registerCAbiCtor*(
     )
   )
 
-proc isStringResp(t: NimNode): bool =
-  t.kind == nnkIdent and ($t == "string" or $t == "cstring")
-
 proc cdeclReplyPragma(): NimNode =
   nnkPragma.newTree(
     ident("cdecl"),
@@ -683,7 +680,8 @@ proc objectTrampBody(boxName, respType, respWire: NimNode): NimNode =
   ## Reply trampoline for an object return: recover the box, deliver a transport
   ## error as a copied NUL-terminated string, else CBOR-decode the reply,
   ## `cwirePack` it into the flat wire struct, hand a pointer to the caller, and
-  ## release the wire.
+  ## release the wire. `err_msg` is always a non-nil string; the `reply` struct
+  ## pointer is nil only on error, gated by a non-`RET_OK` `err_code`.
   quote:
     let box = cast[ptr `boxName`](ud)
     if box.isNil():
@@ -706,15 +704,17 @@ proc objectTrampBody(boxName, respType, respWire: NimNode): NimNode =
       else:
         var wire: `respWire`
         cwirePack(wire, decoded.get())
-        box.fn(RET_OK, addr wire, nil, box.ud)
+        box.fn(RET_OK, addr wire, "".cstring, box.ud)
         cwireFree(wire)
     except CatchableError as e:
       box.fn(RET_ERR, nil, e.msg.cstring, box.ud)
 
 proc stringTrampBody(boxName: NimNode): NimNode =
   ## Reply trampoline for a `string` return (and the ctor's address string):
   ## CBOR-decode the reply into a Nim string and hand its (NUL-terminated)
-  ## `cstring` to the caller for the duration of the call.
+  ## `cstring` to the caller for the duration of the call. Reply and error
+  ## strings are always non-nil empty strings on the paths they don't apply to,
+  ## so a consumer can `strlen`/print either unconditionally without a nil deref.
   quote:
     let box = cast[ptr `boxName`](ud)
     if box.isNil():
@@ -728,16 +728,16 @@ proc stringTrampBody(boxName: NimNode): NimNode =
         var em = newString(int(len))
         if int(len) > 0:
           copyMem(addr em[0], msg, int(len))
-        box.fn(ret, nil, em.cstring, box.ud)
+        box.fn(ret, "".cstring, em.cstring, box.ud)
         return
       let decoded = cborDecodePtr(cast[ptr UncheckedArray[byte]](msg), int(len), string)
       if decoded.isErr():
-        box.fn(RET_ERR, nil, decoded.error.cstring, box.ud)
+        box.fn(RET_ERR, "".cstring, decoded.error.cstring, box.ud)
       else:
         let replyStr = decoded.get()
-        box.fn(RET_OK, replyStr.cstring, nil, box.ud)
+        box.fn(RET_OK, replyStr.cstring, "".cstring, box.ud)
     except CatchableError as e:
-      box.fn(RET_ERR, nil, e.msg.cstring, box.ud)
+      box.fn(RET_ERR, "".cstring, e.msg.cstring, box.ud)
 
 proc exportedMethodProc(
     spec: CAbiSpec, boxName, envWire, trampName, poolIdent, cbType: NimNode
@@ -751,11 +751,18 @@ proc exportedMethodProc(
   let envName = spec.envelope
   let libFFICtx =
     nnkPtrTy.newTree(nnkBracketExpr.newTree(ident("FFIContext"), spec.libType))
+  # A string reply is an empty (non-nil) cstring on the error path, matching the
+  # trampoline; an object reply is a nil struct pointer gated by `err_code`.
+  let emptyReply =
+    if isStringType(spec.respType):
+      newDotExpr(newLit(""), ident("cstring"))
+    else:
+      newNilLit()
   let body = quote:
     if onReply.isNil():
       return RET_MISSING_CALLBACK
     if not `poolIdent`.isValidCtx(cast[pointer](ctx)):
-      onReply(RET_ERR, nil, "ctx is not a valid FFI context".cstring, userData)
+      onReply(RET_ERR, `emptyReply`, "ctx is not a valid FFI context".cstring, userData)
       return RET_ERR
     var reqObj: `envName` = cwireUnpack(req[])
     let enc = cborEncodeShared(reqObj)
@@ -771,10 +778,10 @@ proc exportedMethodProc(
     let sendRes =
       try:
         ffi_context.sendRequestToFFIThread(ctx, reqPtr)
-      except Exception as exc:
-        Result[void, string].err("sendRequestToFFIThread exception: " & exc.msg)
+      except Exception as e:
+        Result[void, string].err("sendRequestToFFIThread exception: " & e.msg)
     if sendRes.isErr():
-      onReply(RET_ERR, nil, sendRes.error.cstring, userData)
+      onReply(RET_ERR, `emptyReply`, sendRes.error.cstring, userData)
       return RET_ERR
     return RET_OK
   newProc(
@@ -815,7 +822,7 @@ proc exportedCtorProc(
       if not onCreated.isNil():
         onCreated(
           RET_ERR,
-          nil,
+          "".cstring,
           ("ffiCtor: failed to create FFIContext: " & $ctxRes.error).cstring,
           userData,
         )
@@ -835,11 +842,11 @@ proc exportedCtorProc(
     let sendRes =
       try:
         ctx.sendRequestToFFIThread(reqPtr)
-      except Exception as exc:
-        Result[void, string].err("sendRequestToFFIThread exception: " & exc.msg)
+      except Exception as e:
+        Result[void, string].err("sendRequestToFFIThread exception: " & e.msg)
     if sendRes.isErr():
       if not onCreated.isNil():
-        onCreated(RET_ERR, nil, sendRes.error.cstring, userData)
+        onCreated(RET_ERR, "".cstring, sendRes.error.cstring, userData)
       return nil
     return cast[pointer](ctx)
   body.insert(0, initGuard)
@@ -899,7 +906,7 @@ proc flushCAbiDispatch*(): NimNode {.compileTime.} =
       sink.add(exportedCtorProc(spec, boxName, envWire, trampName, poolIdent, cbType))
     of cakMethod:
       let rt = spec.respType
-      if isStringResp(rt):
+      if isStringType(rt):
         let cbType = cAbiCbType(ident("cstring"))
         sink.add(boxTypeDef(boxName, cbType))
         sink.add(replyTrampProc(trampName, stringTrampBody(boxName)))