fix: pr comments

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. */
@@ -30,12 +28,8 @@ typedef struct {
 typedef struct {
     ShoutRequest req;
 } EchoShoutReq;
-typedef struct {
-    uint8_t _placeholder; /* C forbids empty structs */
-} EchoVersionReq;
 
 typedef void (*EchoShoutReplyFn)(int err_code, const ShoutResponse* reply, const char* err_msg, void* user_data);
-typedef void (*EchoVersionReplyFn)(int err_code, const char* reply, const char* err_msg, void* user_data);
 
 typedef void (*EchoCreateRawFn)(int err_code, const char* ctx_addr, const char* err_msg, void* user_data);
 #ifdef __cplusplus
@@ -44,7 +38,6 @@ extern "C" {
 
 void* echo_create(const EchoCreateCtorReq* req, EchoCreateRawFn on_created, void* user_data);
 int echo_shout(void* ctx, EchoShoutReplyFn on_reply, void* user_data, const EchoShoutReq* req);
-int echo_version(void* ctx, EchoVersionReplyFn on_reply, void* user_data, const EchoVersionReq* req);
 int echo_destroy(void* ctx);
 
 #ifdef __cplusplus
@@ -60,6 +53,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);
@@ -113,10 +107,4 @@ static inline int echo_ctx_shout(const EchoCtx* ctx, const ShoutRequest* req, Ec
     return echo_shout(ctx->ptr, on_reply, user_data, &ffi_req);
 }
 
-static inline int echo_ctx_version(const EchoCtx* ctx, EchoVersionReplyFn on_reply, void* user_data) {
-    EchoVersionReq ffi_req;
-    memset(&ffi_req, 0, sizeof(ffi_req));
-    return echo_version(ctx->ptr, on_reply, user_data, &ffi_req);
-}
-
 #endif /* NIM_FFI_LIB_ECHO_C_ABI_H_INCLUDED */

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,7 +704,7 @@ 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)
@@ -714,7 +712,9 @@ proc objectTrampBody(boxName, respType, respWire: NimNode): NimNode =
 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)))

ffi/internal/ffi_macro.nim

@@ -1871,8 +1871,7 @@ macro genBindings*(
       )
     of "c_abi":
       generateCAbiBindings(
-        ffiProcRegistry, ffiTypeRegistry, libName, outputDir, nimSrcRelPath,
-        ffiEventRegistry,
+        genProcs, ffiTypeRegistry, libName, outputDir, nimSrcRelPath, ffiEventRegistry
       )
     of "cddl":
       generateCddlBindings(genProcs, ffiTypeRegistry, libName, outputDir, nimSrcRelPath)

tests/e2e/c_abi/test_echo_c_abi.c

@@ -1,10 +1,13 @@
 /* End-to-end test for the CBOR-free `abi = c` echo bindings. Unlike the CBOR C
  * backend, this header links no TinyCBOR: the flat structs in echo.h are the C
  * ABI, strings are plain borrowed `const char*`. The test drives the same
- * async, callback-per-call surface — constructor, an object-returning method,
- * a string-returning method, the error channel and teardown — copying out what
- * each callback delivers (owned by the binding, valid only for the call) and
- * polling a `done` flag to sequence the async calls. */
+ * async, callback-per-call surface — constructor, an object-returning method
+ * and teardown — copying out what each callback delivers (owned by the binding,
+ * valid only for the call) and polling a `done` flag to sequence the async
+ * calls. A string-returning method (echoVersion) rides the CBOR-free scalar
+ * fast path instead of a flat `_CWire` wrapper, so it has no c_abi binding yet
+ * (foreign codegen for the scalar shape is a follow-up) and isn't exercised
+ * here. */
 #include "echo.h"
 #include <assert.h>
 #include <stdatomic.h>
@@ -87,27 +90,9 @@ static void test_shout(EchoCtx* ctx) {
     assert(strcmp(w.text_b, "c-abi") == 0);
 }
 
-static void on_version(int ec, const char* reply, const char* em, void* ud) {
-    ReplyWaiter* w = (ReplyWaiter*)ud;
-    w->err_code = ec;
-    if (reply) snprintf(w->text_a, sizeof(w->text_a), "%s", reply);
-    if (em) snprintf(w->err, sizeof(w->err), "%s", em);
-    atomic_store_explicit(&w->done, 1, memory_order_release);
-}
-
-static void test_version(EchoCtx* ctx) {
-    ReplyWaiter w;
-    memset(&w, 0, sizeof(w));
-    echo_ctx_version(ctx, on_version, &w);
-    wait_done(&w.done);
-    assert(w.err_code == 0);
-    assert(strcmp(w.text_a, "nim-echo v0.1.0") == 0);
-}
-
 int main(void) {
     EchoCtx* ctx = make_ctx();
     test_shout(ctx);
-    test_version(ctx);
     echo_ctx_destroy(ctx);
     printf("all abi=c echo e2e checks passed\n");
     return 0;