cyber icon indicating copy to clipboard operation
cyber copied to clipboard

Printer functions seem to duplicate output prefix

Open AregevDev opened this issue 1 year ago • 3 comments

I am on Windows

Given this C code:

#include <stdio.h>
#include <string.h>

#include "cyber.h"

#define str(x) {x, strlen(x)}

extern "C" {
void printer(CsVM *vm, CsStr str) {
    if (*str.buf != '\n')
        printf("[cyberscript] %.*s\n", (int) str.len, str.buf);
}
}

int main()
{
    CsVM *vm = csCreate();
    csSetPrinter(vm, printer);

    CsValue val;
    CsStr s = str("var a = 2\nprint(a)");
    csEval(vm, s, &val);

    csDestroy(vm);
    return 0;
}

I get the following output

[cyberscript] 2
[cyberscript]

AregevDev avatar Apr 14 '24 11:04 AregevDev

print also adds a new line so it invokes printer twice one for the '2' and another for '\n'. What you could do is check for the newline character and skip it.

You might also want to invoke printf like this: printf("[cyberscript]: %.*s\n", (int)str.len, str.buf); since it doesn't guarantee a zero terminating string.

fubark avatar Apr 14 '24 12:04 fubark

This seems to work

extern "C" {
void printer(CsVM *vm, CsStr str) {
    if (*str.buf != '\n')
        printf("[cyberscript] %.*s\n", (int) str.len, str.buf);
}
}

Maybe cyber should invoke the printer function once. Append the \n to the end of the string instead of calling it twice. I can investigate further and submit a PR for that.

AregevDev avatar Apr 14 '24 16:04 AregevDev

Doing so would either need a fixed buffer where longer strings would get truncated or an extra heap allocation. Neither is a great choice IMO.

I think it's the right abstraction. The printer behaves like a write(ptr, len) interface for stdout similar to the error printer for stderr. I think what would be better is to allow core.print to be replaced from the C-API.

Something like this:

bool onLoad(CLVM* vm, CLModule mod) {
  CLSym sym = clGetSym(mod, "print");
  CLFuncSym func = clGetFirstFunc(sym);
  clSetHostFunc(func, my_print);
}
CLValue my_print(CLVM* vm, const CLValue* args, uint8_t nargs) {
    // str = toString with newline
    clPrint(vm, str);
}

And onLoad would be registered in the module loader, but we'd also need to expose the core modules configuration so that just the onLoad can be replaced.

fubark avatar Apr 14 '24 16:04 fubark