Added a function to print to pre-allocated buffer #72

ChisholmKyle · 2016-11-25T10:14:43Z

I added a function called cJSON_PrintMallocedBuffer to print output to a buffer instead of allocating memory internally. I'm made it so there aren't any calls to malloc when printing out a cJSON item.

I also fixed the Makefile to have the appropriate dependencies and only source files (no headers) being compiled for the tests. Otherwise, clang on Mac (and perhaps elsewhere) complains.

FSMaxB · 2016-11-25T12:05:39Z

cJSON.c

+    p.buffer = buf;
+    p.length = len;
+    p.offset = 0;
+    out = print_value(item,0,fmt,&p);


By doing this you risk crashes, because print_value will try to reallocate the printbuffer when it is too short.

FSMaxB · 2016-11-25T12:28:06Z

I understand why a function like that can be useful, but the implementation should be more carefully considered.

The name should be more like cJSON_PrintPreallocated or something like that. Also we haven't done the switch to size_t, but it is on it's way in cJSON 2.0.

If you don't do multithreading, there is a (quite hacky, I admit that) workaround to do what you want on the current version of cJSON btw.

/* consider this as pseudo code because I haven't actually compiled and tested it */
static int run_already = 0;
static char *buffer = NULL;
static size_t buffer_length = 0;

void *allocator(size_t size)
{
    if (run_already || (size > buffer_length))
    {
        return NULL;
    }
    run_already = 1;

    return buffer;
}

void deallocator(void *buffer)
{
    return;
}

cJSON_Hooks static_allocators = { allocator, deallocator };

int main(void)
{
    /*... */
    char local_buffer[100];
    buffer = local_buffer;
    buffer_length = 100;
    run_already = 0;

    cJSON_InitHooks(&static_allocators);

    cJSON_PrintBuffered(item, 100, 0);

    buffer = NULL;
    buffer_length = 0;
    run_already = 0;
    /* ... */

    return 0;
}

Yes, it's really ugly ...

If one would actually implement this functionality in cJSON, it would probably be best to add a flag to printbuffer that stores if the buffer is to be considered constant and then modifying ensure to handle that case.

We can have a discussion at least!

ChisholmKyle · 2016-11-25T18:43:58Z

Seems like quite a bit of of user code to perform what seems like a simple idea. I really like idea of adding a flag to the printbuffer struct. I made the change and added a small test.

I found a bug: If the supplied buffer was too small by 1 or 2 bytes, the last closing curly bracket is omitted and no error is detected. I made a change to the source in the last commit which adds a check for a NULL pointer returned from print_value.
This seems to fix the problem, but the tests are not automated and I'm not sure it's worth inspecting the output manually. I'm also not as familiar with the source.

FSMaxB · 2016-11-26T05:21:55Z

cJSON.c

@@ -244,8 +244,10 @@ typedef struct
    char *buffer;
    int length;
    int offset;
+    int noalloc;


Please use cjbool and use true and false when assigning to it.

FSMaxB · 2016-11-26T05:22:21Z

cJSON.c

 } printbuffer;

+


Don't add random whitespace.

FSMaxB · 2016-11-26T05:24:47Z

cJSON.c

+    p.offset = 0;
+    p.noalloc = 1;
+    out = print_value(item,0,fmt,&p);
+    return (out != buf ? -1 : 0);


This should return out like the other print functions. It will be NULL if it failed.

If this function returns a char pointer (effectively providing an alias to the input char *buf or a NULL pointer), it might falsely imply a new dynamically allocated string was created. Do you think it might be more intuitive to use the pointer to char* buf after the function is called if no error was returned, or is consistency of return values with all the cJSON_Print functions more important?

Hm. Good point. Considering this, your way of returning true or false might actually be better. An API should be hard to misuse in a dangerous way (like having a double free because of the alias).
The question is how far one is willing to protect the users from themselves.

This will then not allow the use of cJSON_PrintPreallocated as an argument to another function (do_something(cJSON_PrintPreallocated(...)))

If you do it the way you suggested, you can just return print_value(...) != NULL;

That would be more concise. Made the change.

FSMaxB · 2016-11-26T05:28:28Z

cJSON.c

+            if (!ptr)
+            {
+                return NULL;
+            }


Change this to:

if (!print_value(child, depth + 1, fmt, p)) { return NULL; }

Thank you for finding this problem btw. after that I was taking a closer look to the rest of the library and there are quite some cases where out of memory errors are not handled properly.

I will also fix these other cases in the next release.

FSMaxB · 2016-11-26T05:31:43Z

cJSON.c


    return print_value(item, 0, fmt, &p);
 }

+int cJSON_PrintPreallocated(cJSON *item,char *buf, const size_t len, const cjbool fmt)


About the size_t. I agree that size_t should be used everywhere instead of int, and it will be in v2, but currently ensure and printbuffer and all the other internal functions use int for sizes, so using size_t can create the illusion that numbers exceeding the integer range are supported, but they're not.

So please change to int 😢.

FSMaxB · 2016-11-26T05:43:56Z

test.c

+
+    printf("Test cJSON_PrintPreallocated:\n");
+    /* create buffer */
+    size_t len = strlen(out) + 1;


It is not allowed to mix code and declarations in C89. I have a bug in the Makefile and CMakelists.txt so that the compiler option is not enforced, sorry for that.

I suggest you create a new function just for testing preallocated printing and call it from main (you can pass one of the texts to it).

ChisholmKyle · 2016-11-27T00:08:49Z

Thanks for the thorough feedback, I'll make the requested changes soon :)

ChisholmKyle · 2016-11-28T09:01:00Z

Okay made the changes. Let me know what you think.

FSMaxB · 2016-11-28T15:34:35Z

Makefile

@@ -23,7 +23,7 @@ INSTALL_LIBRARY_PATH = $(DESTDIR)$(PREFIX)/$(LIBRARY_PATH)

 INSTALL ?= cp -a

-R_CFLAGS = -fPIC -std=c89 -pedantic -Wall -Werror -Wstrict-prototypes -Wwrite-strings $(CFLAGS)
+R_CFLAGS = -fPIC -std=c89 -pedantic -Wall -Werror -Wstrict-prototypes -Wwrite-strings -Wshadow -Winit-self -Wcast-align -Wformat=2 -Wmissing-prototypes $(CFLAGS)



Please don't try to readd changes that I merged to the master branch already. I will handle merge conflicts myself.

FSMaxB · 2016-11-28T15:43:06Z

cJSON.c

+    p.length = len;
+    p.offset = 0;
+    p.noalloc = true;
+    return (print_value(item,0,fmt,&p) != NULL ? 0 : -1);


I meant it exactly the other way around. Think of the return value as a bool. So return print_value(...) != NULL. It returns 1 if it succeeds and 0 if it doesn't.

FSMaxB · 2016-11-28T15:43:37Z

I think I will manually merge in your changes.

FSMaxB · 2016-11-28T16:33:23Z

I merged your changes, see bf17703

Kyle Chisholm added 3 commits November 25, 2016 04:53

added Print function call for pre-allocated buffer

2387d9d

fixed "make test" on Mac (clang)

1193d24

fix shared utils library to also include cjson object

df093c3

FSMaxB reviewed Nov 25, 2016

View reviewed changes

FSMaxB added the undecided label Nov 25, 2016

changed to cJSON_PrintPreallocated, added flag in printbuffer

42496b2

FSMaxB requested changes Nov 26, 2016

View reviewed changes

FSMaxB removed the undecided label Nov 27, 2016

Kyle Chisholm added 8 commits November 28, 2016 03:19

change noalloc struct item to cjbool

4150c30

check print_value return

6ce0915

function to print and compare to preallocated buffer

6e1b4ba

use print_preallocated function to test printout of JSON

652bb77

change buffer length to to int

e559516

more concise return

e3e7726

forgot to add 0, -1 return values

32f3316

fix flags

7083240

FSMaxB reviewed Nov 28, 2016

View reviewed changes

FSMaxB closed this Nov 28, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added a function to print to pre-allocated buffer #72

Added a function to print to pre-allocated buffer #72

ChisholmKyle commented Nov 25, 2016

FSMaxB Nov 25, 2016

FSMaxB commented Nov 25, 2016 •

edited

Loading

ChisholmKyle commented Nov 25, 2016 •

edited

Loading

FSMaxB Nov 26, 2016

FSMaxB Nov 26, 2016

FSMaxB Nov 26, 2016

ChisholmKyle Nov 28, 2016

FSMaxB Nov 28, 2016

ChisholmKyle Nov 28, 2016

FSMaxB Nov 26, 2016

FSMaxB Nov 26, 2016

FSMaxB Nov 26, 2016

FSMaxB Nov 26, 2016

ChisholmKyle commented Nov 27, 2016

ChisholmKyle commented Nov 28, 2016

FSMaxB Nov 28, 2016

FSMaxB Nov 28, 2016

FSMaxB commented Nov 28, 2016

FSMaxB commented Nov 28, 2016

Added a function to print to pre-allocated buffer #72

Added a function to print to pre-allocated buffer #72

Conversation

ChisholmKyle commented Nov 25, 2016

Choose a reason for hiding this comment

FSMaxB commented Nov 25, 2016 • edited Loading

ChisholmKyle commented Nov 25, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChisholmKyle commented Nov 27, 2016

ChisholmKyle commented Nov 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FSMaxB commented Nov 28, 2016

FSMaxB commented Nov 28, 2016

FSMaxB commented Nov 25, 2016 •

edited

Loading

ChisholmKyle commented Nov 25, 2016 •

edited

Loading