Change API to use string+len pairs instead of null terminated strings

[Hejsil]

Implements this issue.

This pull request doesn't actually implement this. It only contains changes to the header files. This is my proposed new API for strings in the Nuklear library. I would love to hear some feedback on this.

Notes

• Adding new struct called nk_slice (nk_str and nk_text was taken). This is the string+len pair.
  • Existing code can make an nk_slice from a const char * by calling nk_slice("str").
• The format string versions of nk_label cannot really be changed without throwing away the NK_PRINTF_VARARG_FUNC attribute. I've therefore chosen to leave these functions as is.
• There is no longer both a nk_*_label and nk_*_text variant of varies functions. nk_*_label is the naming convention that was kept, but these functions now take an nk_slice.

Benifits

• This pattern allows for more optimal code. This allows for any buffer to be passed to these functions, as all we require is a pointer and a length.
  • This allows users to avoid allocations in certain cases.
• Other languages have chosen not to have null terminated string by default, and these languages can now have more efficient bindings.
• We remove a decent amount of API surface area as we already had a decent amount of function which take a string + len.

Cons

• This is a major breaking change.
• Using Nuklear from C is a little more verbose.

[wheybags]

Here's an idea for a potential ergonomic improvement for c++ users (which I imagine is most people?) When compiling as c++, add a constructor from std::string_view on the nk_slice struct, so then the change is pretty much transparent.

Eg:

struct nk_slice 
{
    const char *ptr;
    nk_size len;
#ifdef __cplusplus
    nk_slice(std::string_view view) : ptr(view.data()), len(view.length) {}
#endif
};

If you're not comfortable using c++17, it could be an std::string& either.

[dumblob]

@wheybags if the proposal is so much dependent on C++ version, shouldn't we rather document it instead of providing it by default in the code? Or maybe implement just a tiny subset which really overlaps between all C++ versions?

[wheybags]

That is why I suggested using an std::string reference instead, that would work on all versions. Alternatively, you could even detect versions:

struct nk_slice 
{
    const char *ptr;
    nk_size len;
#ifdef __cplusplus
#  if __cplusplus >= 201703L // if we have c++17, use string view
    nk_slice(std::string_view view) : ptr(view.data()), len(view.length()) {}
#  else // otherwise fall back to a string ref
    nk_slice(std::string& str) : ptr(str.c_str()), len(str.length()) {}
#  endif
#endif
};

[dumblob]

@wheybags hm, I've heard std:: is not recommended e.g. for game development due to its performance impact - do you think it's worth introducing such dependency? Or would we need another #define to disable this dependency?

[wheybags]

I work on a commercial game, and we use the STL heavily. That said, a lot of studios prefer not to. Even among those which don't, I would be surprised to find them not using std::string at all. If someone doesn't want to use it, they can just not use it, by explicitly initialising the nk_slice using some alternative method. Even if they aren't using the STL, they will still have the headers available, so it should compile just fine.

[Hejsil]

Here is my personal take. Nuklear is a C library, not a C++ library. I does the bare minimum to ensure that the library can compile under C++ but nothing more and I think it should stay this way.

std is just a library like any others. What makes std::string_view special from llvm::StringRef, absl::string_view, boost::string_view and so on. Currently Nuklear has no dependencies (even libc can be avoided as far as I know).

There are also a none trivial problem introduced by adding a constructor. An initializer list can no longer be used to initialize this struct:

struct nk_slice {
    const char *ptr;
    int len;

    nk_slice(const char *ptr) : ptr(ptr), len(0) {}
};

int main() {
    struct nk_slice s = { "t", 1 };
}

test.cpp:10:34: error: could not convert ‘{"t", 1}’ from ‘<brace-enclosed initializer list>’ to ‘nk_slice’
   10 |     struct nk_slice s = { "t", 1 };
      |                                  ^
      |                                  |
      |                                  <brace-enclosed initializer list>

So now, our valid C code does not compile under a C++ compiler.

[wheybags]

Well, maybe things like this should live in a separate header. In general, it would be nice to see more ergonomic c++ usage. A supplementary nuklear.hpp could do that.

[Hejsil]

Also, c++ have it easy compared to other languages when it comes to simple usage. :wink:

#include <cstddef>

struct nk_slice {
    const char *ptr;
    size_t len;
};

struct nk_slice operator "" _nk(const char *ptr, size_t len) {
    return {ptr, len};
}

int main() {
    struct nk_slice s = "test"_nk;
}

[Hejsil]

In general, it would be nice to see more ergonomic c++ usage. A supplementary nuklear.hpp could do that.

Well, this sounds like bindings to me, which people already maintain in various languages

[Hejsil]

Anyways, I don't think this is to relevant for this specific PR and is something that could be added later anyways. This change would already make the library a little more ergonomic for c++ usage.

[rswinkle]

I'm not sure I'm understanding your last two Notes bullet points. In the changes I didn't see any nk_*_label functions changed/removed?

Also it looks like all the functions that changed were internal functions, not something I'd call directly from my application GUI code. I hope that's true because I think it'd be sad if C programmers using a C library couldn't do what comes easily and naturally in C, ie pass string literals directly as const char*. Having to do something like this:

struct nk_slice s = { "My Label",  8 };
nk_label(ctx, s, NK_TEXT_LEFT);

would be bad enough. That would be the most efficient way but having to manually count would be annoying and introduce many off-by-1 errors (probably quickly obvious and fixable but still). Compound literals (if you were willing to use them despite the C/C++ issues) would cut a line but leave the counting problem.

On the other hand, encouraging C programmers to have dozens or hundreds of little heap allocations for constant (and even update per frame type) strings hurts my soul and is longer.

sds s = sdsnew("My Label");
struct nk_slice label = { s, sdslen(s) }
nk_label(ctx, label, NK_TEXT_LEFT);

So, if at all possible, I say keep the interface that let us pass string literals and let the language bindings wrap inner functions that take an nk_slice or equivalent. That's just my 2 cents for what it's worth.

[Hejsil]

I'm not sure I'm understanding your last two Notes bullet points. In the changes I didn't see any nk_*_label functions changed/removed?

Also it looks like all the functions that changed were internal functions, not something I'd call directly from my application GUI code.

This PR removes the nk_*_text variants and makes the nk_*_label variants of functions take the nk_slice. These changes are not just internal, but changes to the API, as you can see from this diff:

-NK_API void nk_text(struct nk_context*, const char*, int, nk_flags);
-NK_API void nk_text_colored(struct nk_context*, const char*, int, nk_flags, struct nk_color);
-NK_API void nk_text_wrap(struct nk_context*, const char*, int);
-NK_API void nk_text_wrap_colored(struct nk_context*, const char*, int, struct nk_color);
-NK_API void nk_label(struct nk_context*, const char*, nk_flags align);
-NK_API void nk_label_colored(struct nk_context*, const char*, nk_flags align, struct nk_color);
-NK_API void nk_label_wrap(struct nk_context*, const char*);
-NK_API void nk_label_colored_wrap(struct nk_context*, const char*, struct nk_color);
+NK_API void nk_label(struct nk_context*, struct nk_slice, nk_flags align);
+NK_API void nk_label_colored(struct nk_context*, struct nk_slice, nk_flags align, struct nk_color);
+NK_API void nk_label_wrap(struct nk_context*, struct nk_slice);
+NK_API void nk_label_colored_wrap(struct nk_context*, struct nk_slice, struct nk_color);

I hope that's true because I think it'd be sad if C programmers using a C library couldn't do what comes easily and naturally in C, ie pass string literals directly as const char*. Having to do something like this:

struct nk_slice s = { "My Label",  8 };
nk_label(ctx, s, NK_TEXT_LEFT);

would be bad enough. That would be the most efficient way but having to manually count would be annoying and introduce many off-by-1 errors (probably quickly obvious and fixable but still). Compound literals (if you were willing to use them despite the C/C++ issues) would cut a line but leave the counting problem.

On the other hand, encouraging C programmers to have dozens or hundreds of little heap allocations for constant (and even update per frame type) strings hurts my soul and is longer.

sds s = sdsnew("My Label");
struct nk_slice label = { s, sdslen(s) }
nk_label(ctx, label, NK_TEXT_LEFT);

So, if at all possible, I say keep the interface that let us pass string literals and let the language bindings wrap inner functions that take an nk_slice or equivalent. That's just my 2 cents for what it's worth.

Alright, this hits a lot of points. If you are a user of the API, most of the times your code will be updated like this:

-nk_label(ctx, "test", NK_TEXT_LEFT);
+nk_label(ctx, nk_slice("test"), NK_TEXT_LEFT);

This is not really that bad for a C programmer. It looks pretty clean. Now, you mention a point about efficiency:

struct nk_slice s = { "My Label",  8 };
nk_label(ctx, s, NK_TEXT_LEFT);

That would be the most efficient way but having to manually count would be annoying and introduce many off-by-1 errors (probably quickly obvious and fixable but still). Compound literals (if you were willing to use them despite the C/C++ issues) would cut a line but leave the counting problem.

Yes, this would be the most efficient way. However let me just point out that the old API called strlen internally. This means, that nuklear already did the work of calling strlen, so having to call nk_slice is not any less efficient than the old API. The key here is, that with the new API the user can be more efficient. As you yourself show, you can now avoid the strlen entirely if you know the length ahead of time.

Now, I don't recommend people counting the length of the string and manually inserting it in their code. The best thing you can do in C is to have a macro for this (there are some footguns with this approach, but it is efficient).

#define NK_SLICE(str) (struct nk_slice){str, sizeof(str)-1}
nk_label(ctx, NK_SLICE(s), NK_TEXT_LEFT);

On the other hand, encouraging C programmers to have dozens or hundreds of little heap allocations for constant (and even update per frame type) strings hurts my soul and is longer.

sds s = sdsnew("My Label");
struct nk_slice label = { s, sdslen(s) }
nk_label(ctx, label, NK_TEXT_LEFT);

And I agree. We should avoid allocations. But with the new API you can more easily do this. Imagine something like this:

char *user_input = ...;

// We can now slice the string, without writing '\0' to it or duplicating it.
struct nk_slice sliced = { user_input + 3, 3 };
nk_label(ctx, sliced, NK_TEXT_LEFT);

There was no way to do this for some functions with the old API (yes, nk_label had a nk_text variant, but not all functions had this).

So, if at all possible, I say keep the interface that let us pass string literals and let the language bindings wrap inner functions that take an nk_slice or equivalent. That's just my 2 cents for what it's worth.

This change is is trying to streamline the API, making it more efficient and reduce the surface area. I can flip this question around. Imagine Nuklear was written like this in the first place, and someone wanted to add wrappers to Nuklear that allowed users to pass null terminated strings. I would then ask, should we also have wrappers for wchar_t, u32 (utf32 chars) or maybe newline terminated strings? Yes, C programmers have used null terminated strings for ages, but they have a lot of problems. Wouldn't it be better to expose a more efficient, more powerful and less error prone API and let the user wrap this API to their needs?

[rswinkle]

This PR removes the nk_*_text variants and makes the nk_*_label variants of functions take the nk_slice. These changes are not just internal, but changes to the API, as you can see from this diff:

-NK_API void nk_text(struct nk_context*, const char*, int, nk_flags);
-NK_API void nk_text_colored(struct nk_context*, const char*, int, nk_flags, struct nk_color);
-NK_API void nk_text_wrap(struct nk_context*, const char*, int);
-NK_API void nk_text_wrap_colored(struct nk_context*, const char*, int, struct nk_color);
-NK_API void nk_label(struct nk_context*, const char*, nk_flags align);
-NK_API void nk_label_colored(struct nk_context*, const char*, nk_flags align, struct nk_color);
-NK_API void nk_label_wrap(struct nk_context*, const char*);
-NK_API void nk_label_colored_wrap(struct nk_context*, const char*, struct nk_color);
+NK_API void nk_label(struct nk_context*, struct nk_slice, nk_flags align);
+NK_API void nk_label_colored(struct nk_context*, struct nk_slice, nk_flags align, struct nk_color);
+NK_API void nk_label_wrap(struct nk_context*, struct nk_slice);
+NK_API void nk_label_colored_wrap(struct nk_context*, struct nk_slice, struct nk_color);

Ah, I didn't expand the diff on nuklear.h because I was thinking it was the generated nuklear.h so the changes would be the same as the other files. Forgot that there was a src/nuklear.h as well. My bad.

Alright, this hits a lot of points. If you are a user of the API, most of the times your code will be updated like this:

-nk_label(ctx, "test", NK_TEXT_LEFT);
+nk_label(ctx, nk_slice("test"), NK_TEXT_LEFT);

This is not really that bad for a C programmer. It looks pretty clean. Now, you mention a point about efficiency: Ok that's not too bad; should have looked at that diff and I'd have realized what you were going for. While it works here because nuklear doesn't typedef its structs (one of the few things I disagree with Linus Torvalds about, that and 8 space tabs yikes), in general it's weird to see what looks like a constructor in C.

Yes, this would be the most efficient way. However let me just point out that the old API called strlen internally. This means, that nuklear already did the work of calling strlen, so having to call nk_slice is not any less efficient than the old API. The key here is, that with the new API the user can be more efficient. As you yourself show, you can now avoid the strlen entirely if you know the length ahead of time.

Yeah, I was aware of that. Again this all stems from my not seeing the majority of the changes. Though I don't consider strlen a significant overhead in any case, especially for strings that are usually dozens or at most 100's of bytes. I was comparing to the performance of the added heap operations suggested below which are much slower.

Now, I don't recommend people counting the length of the string and manually inserting it in their code. The best thing you can do in C is to have a macro for this (there are some footguns with this approach, but it is efficient).

#define NK_SLICE(str) (struct nk_slice){str, sizeof(str)-1}
nk_label(ctx, NK_SLICE(s), NK_TEXT_LEFT);

This is probably the best alternative or at least tied with calling nk_slice(), but as you said, there are cases where str isn't an array so sizeof won't work and we have the compound literal I discussed in another paragraph.

And I agree. We should avoid allocations. But with the new API you can more easily do this. Imagine something like this:

char *user_input = ...;

// We can now slice the string, without writing '\0' to it or duplicating it.
struct nk_slice sliced = { user_input + 3, 3 };
nk_label(ctx, sliced, NK_TEXT_LEFT);

There was no way to do this for some functions with the old API (yes, nk_label had a nk_text variant, but not all functions had this).

I feel like this is a rare use case but I can appreciate shrinking the API while gaining functionality

This change is is trying to streamline the API, making it more efficient and reduce the surface area. I can flip this question around. Imagine Nuklear was written like this in the first place, and someone wanted to add wrappers to Nuklear that allowed users to pass null terminated strings. I would then ask, should we also have wrappers for wchar_t, u32 (utf32 chars) or maybe newline terminated strings? Yes, C programmers have used null terminated strings for ages, but they have a lot of problems. Wouldn't it be better to expose a more efficient, more powerful and less error prone API and let the user wrap this API to their needs?

I get your point, though isn't it UTF-8 by default already and it works as long as the nuklear backend (and font) you're using supports it? I feel that's pretty universal, probably the most popular, and when people are using other rarer character encodings, they're usually having to convert one way or another between interfaces/libraries.

Lastly, I will admit that while I'm kind of unique in my programming experience and preferences, I've never really had problems with null-terminated strings. Like everyone else, I've written my own wrapper/dynamic string type when I needed more power, but in and of themselves, I don't see plain old C-strings as particularly error prone.

[dumblob]

Didn't have time to take a look at this rather bigger PR, but I want to say thanks for this very good discussion. Feel free to discuss it further as this will be quite a big thing for Nuklear in general.

Judging based on the discussion so far, I really like the implementation approach.

Uneducated suggestion: some of the points raised by @rswinkle could be explained in comments in a more direct way (basically formulated as "answers" to @rswinkle's questions) instead of just in this discussion.

@hejsil if you feel we should merge the PR any time soon (days/weeks), please raise your hand as I have currently a lot of other high-priority work to do and would need to reschedule things. And of course, feel free to "recruit" new reviewers to speed things up :wink:.

[Hejsil]

Uneducated suggestion: some of the points raised by @rswinkle could be explained in comments in a more direct way (basically formulated as "answers" to @rswinkle's questions) instead of just in this discussion.

So having comments explaining why we have nk_slice, what it is used for and how to use it? We could document the nk_slice struct itself. This is probably the first place people would look when seeing a signature containing it:

/* `nk_slice` is the data structure Nuklear uses to pass around strings (in favor of `const char*`).
 * This struct carries around both the pointer to bytes and the number of bytes pointed to. Using
 * `nk_slice` over a null terminated `const char*` allows Nuklear to avoid calling `strlen`, makes
 * the Nuklear API safer, and allows bindings from other languages to be more efficient when
 * these languages don't use null terminated strings. `nk_slice` also allows the user to slice up
 * a string into multiple parts without allocation, allowing C users to be more efficient as well.
 * To get an `nk_slice` from a null terminated string, use the `nk_slicez` function.
 */
struct nk_slice { const char *ptr; nk_size len; };

if you feel we should merge the PR any time soon (days/weeks), please raise your hand as I have currently a lot of other high-priority work to do and would need to reschedule things. And of course, feel free to "recruit" new reviewers to speed things up wink.

This will probably take a while (weeks maybe idk), a there is a lot that needs to be changed. It's mostly just a straight forward refactor, so it's just about taking time to getting it done.

[Hejsil]

Just put in the last work required to get the entire library compiling. Haven't tested anything yet, but I feel pretty confident about this PR now that I have the entire picture of how the library looks now.

Next step is to get all demoes and examples compiling!

[Hejsil]

I now have all examples and demos that I can compile on my linux machine working. The primary code in nuklear.h is fully ready for review if someone feel brave enough to jump in. I do plan on looking over all my changes one more time.

TODO:

• Get non linux backends working (anyone know how I can easily try to compile the glad backend)
• Run and test all demos
• Finish demo/node_editor.c,style.c,calculator.c

[Hejsil]

Once I feel I'm done, I'll rebase on master, bump the major version number and squash most commits together

[dumblob]

Get non linux backends working (anyone know how I can easily try to compile the glad backend)

If it's just about compilation (not "visual" quality&functionality), then GitHub CI (or other CI of your choice - GitHub CI is though extremely fast - we're talking seconds instead of minutes as with e.g. Travis CI) might help. I didn't have time for that, but if you feel it's a good thing, then we might adopt something similar likehttps://github.com/vlang/v/blob/master/.github/workflows/ci.yml .

[Hejsil]

@dumblob Aah, ofc. Idk why I didn't think of using CI for this. I've had good experience with Github CI, so I'll try that out tomorrow.

[Hejsil]

Done! All demoes have been compiled and tested. Commits have been squashed. Branch has been rebased on master. I'll take one more look over the changes to make sure I did everything right. After that I'll bump the version and this should be ready for merge.

[Hejsil]

Done!

[lundril]

Just my 2 cents: This really is a big API change. I still like it: There are so many potential problems with the NUL terminated C strings, that using something else really is nice.

[Hejsil]

@ronaaron @Michael-Kelley @rswinkle I'm pinging a few people here how had (or have) PR's that add a less trivial amount of code to the Nuklear codebase. I'm looking to get this PR merge but I need at least one person to look over the code in case I missed anything. You any of you don't have time, that is perfectly fine. I'm just reaching out to see if we can get this reviewed and merged in a timely fasion.

Also we (the Immediate-Mode-UI organization) are looking for more members that would be interested in reviewing PR for Nuklear. @dumblob seem to be busy, so having one more person would be very helpful. Any interest here?

[ronaaron]

Thanks for pinging me. I am actually very happy about the idea behind this, but I don't have time at the moment to review it. If it's still 'on the ropes' in a week or so, I'll dive in.

[rswinkle]

I might be able to look over it next week but my pull request, while a decent size, didn't actually touch Nuklear proper. It was/is just a big demo. The bug I fixed last year was actually really trivial and just involved finding the regression and working around it.

All this to say that I'm not sure how useful my reviewing the changes would be but I'll let you know if I do.

[Hejsil]

@rswinkle I don't think any real internals changed in this PR. It is mostly changes an API change, with all the code changed to compile under this new API. I mostly need a look over to spot if I made any mistakes converting ptr + offset into nk_substr(slice,offset,slice.len) and stuff like it.

Anyways, it is up to you if you feel you can contribute a meaningful review :+1:

[dumblob]

I'll chime in despite not making my hands dirty with the code yet (the following caught my eye when skimming over my inbox).

As an aside, it's weird to me to think of passing things that don't fit in a single register by value. I know an nk_slice would be 16 bytes on a 64-bit machine but I've never thought about, tested, or looked up how the calling conventions handle something like that. Would it split it across 2 registers or drop to the stack even if it hasn't used up the 6 registers designated for parameters? Probably the former in a case like this but if it was large enough to overflow the remaining registers that would be awkward to have it half in registers and half in stack. Meh, I know it's irrelevant here.

Due to the enormously wide range of targets Nuklear tries to support, this indeed seems irrelevant - just look at those ubiquitous ARMs with their totally weird mixture of parameter passing - the strategy differs a lot and is too unstable to do any generic optimization (which args - depends on their type - in which order... without any logic - you probably can end up with first arg on stack, second register, next 10 args stack, then 3 args registres, etc.).

I'd leave all this completely up to the compiler. Besides having args in registers is no win in general on modern CPUs (this holds also for embedded ones) because of extreme interleaving of instructions and huge caches. So args in registers can easily become slower than if on stack in case one tries to "optimize".

And yes, making nk_substr static is fine for me.

[rswinkle]

@dumblob I know, modern architectures are crazy with caches, out-of-order execution, speculation (for better or worse) and long pipelines.

I'm coming from the perspective of having done a decent number of small to medium assembly programs in the last few years in MIPS, RISC-V and some x86_64. I know there's not a clear performance case for small structures but from an assembly programmer's convenience/readability case, registers beat the stack imo. I know people loved the doubling of registers from 32 to 64 bit that let them change the calling conventions for x86 (and spawned the x32 ABI as well).

Combine that with learning a long time ago to avoid passing "large" structures by value to avoid copy overhead (which for me became almost never even for smaller structs) and that's why it seems weird to me.

On the subject of ARM, I don't have any practical experience but from what I've seen of it, it's closer to the complexity of x86 than MIPS/RISC-V even though it's technically RISC. And like you said it's become so varied and has so many versions over the last decade or 2 I just haven't had the incentive to learn it. I hope RISC-V grows and becomes more popular. The openness is cool* but it's just so nice as an assembly programmer (and I imagine for a compiler target as well).

Sorry to go on a tangent ITT.

• I know POWER is "more" open apparently, and now MIPS is open too.

[dumblob]

If someone's interested, here is one prominent example how stack & heap (but using more of out-of-order, speculative processing, ...) beat registers (with less out-of-order, less speculative processing, ...): Hoare’s rebuttal and bubble sort’s comeback .