Comments (16)
If you can provide a PR for this one that would be great! Thanks again for all the reports, I appreciate the help!
from zpl.
@zpl-zak - a significant trickiness to the change is the problem occurs a lot in _init
calls which all have the same design pattern of being a large do/while in a #define - there isn't really a good way to return failure.
I'm minded to suggest I add _alloc
functions which return the type being created, similar to zpl_adt_alloc
.
I guess that would then mean the _init
calls could be marked deprecated but remain (for now/ever) for API stability.
I didn't anticipate causing that much of a change! I am very open to any suggestions from you regarding which direction I set of in.
from zpl.
Sadly that would be difficult to change as many other modules rely on an allocation that would be affected.
However, since we know this issue is related to containers (zpl_array, zpl_buffer), I suggest we expand the header structure of these containers and add an error code field that would get set in case we fail any allocation. Users can then refer to this field to check for OOM errors and handle them appropriately, which is a viable alternative to returning by error code.
The user, in this case, would be the parser/adt code that would then validate this error code and return the appropriate error to the library user.
EDIT: I can address this tomorrow, and I will use your demo code as a test case to validate the bad path in our UTs.
Once I get the branch ready, we can discuss if this is a viable path forward.
from zpl.
I'm not sure the knock-on effect is too bad. I guess if zpl ends up checking NULL everywhere, each module is getting updated one way or another. Neither approach requires that to happen today, I was just trying to leave a pattern that could be used each time later on.
Here is the proposed change master...rheatley-pervasid:zpl:add-alloc-call
I couldn't see a good reason why the array held onto the original pointer value, so removed it for simplicity.
(There is an unnecessary fix to zpl_strdup
in there as well)
from zpl.
I see, yes this approach would work well in that case. I agree performing the alloc in the macro is flawed due to no error handling being present, luckily with your approach, it shouldn't break backward compatibility indeed. Let's continue going down this path.
from zpl.
I think arrays are complete
master...rheatley-pervasid:zpl:add-alloc-call
Because I cannot rely on sizeof() for the array type, elem_count has got cached.
I couldn't understand how capacity
could ever be less than count
which was a special case in array.c
If you agree, the recursive call is unnecessary and I inlined the function.
If you are still happy with this direction I will progress it over the next week.
from zpl.
Looks good to me; thanks again for the contribution! ๐๐ป
from zpl.
Been a lot busier than anticipated, so a bit delayed finishing off the arrays work.
I came to the conclusion that API pattern wouldn't work well when the array got reallocated underneath us.
So I've had a second go, master...rheatley-pervasid:zpl:alloc-take-2
Essentially all existing "functions" are now really functions and return true if success, false if allocation failed.
zpl_array_append
and zpl_array_append_at
are the least nice in terms of implementation. So let me know if too magical!
p.s. there is at least a bug in zpl_array_appendv_at
not involving ind
in the zpl_memmove
number of bytes to move
from zpl.
I think this approach will work out fine in the end. I like how things look so far, thank you again!
from zpl.
Hi @zpl-zak - been quite busy so this got a bit neglected.
I think I am happy with where I left it last - master...rheatley-pervasid:zpl:alloc-take-2
Would you like anything tidying up or I'll create a PR? json.c looks to have had a lot of whitespace in, which my editor stripped. I can restore it if you want a more minimal diff.
from zpl.
Hi @rheatley-pervasid, I will get back to it this upcoming weekend. Sorry for the delay!
from zpl.
@rheatley-pervasid Sorry again for disappearing! I think it looks fine, could we also expand it to zpl_buffer and other macro-based collections? If not that's fine too, we can go ahead with PR and I can follow your changes and apply them to other collections. Thanks for the contrib!
from zpl.
@zpl-zak no worries, it seemed optimistic to look at it much over Christmas :)
I'm very happy to keep working through other non-ideal allocs. I'm tempted to say I'll make a PR for this in isolation
- if I have broken anything, spotting it in a smaller commit should be easier
- selfishly the JSON allocations are more time critical for our project
(we still use the buffers elsewhere in the code, so fixing still on my radar!)
from zpl.
Hi, I'm sorry I am no longer current on this issue. Are we good to close it now, or are more actions required?
from zpl.
Hi @zpl-zak - it is up to you really.
There are still several locations where zpl_alloc() is not checked (string.h, buffer.h) - but I think I fixed all the cases that could occur from JSON serialisation/parsing.
So either, it can be a placeholder for the other issues, they can get their own issue, or it is all forgotten about for a bit!
(I was fixing some more stuff here, https://github.com/rheatley-pervasid/zpl/commits/alloc-improvements - but I've been very short of time recently, so I doubt I will make any more progress in the short term)
from zpl.
Thanks for the response! I will look into the mentioned branch, apply the fixes to the main repo, and continue down this path to cover more locations. Thank you for the help so far. No worries, it does not hurry. I'll keep this issue open for tracking purposes.
from zpl.
Related Issues (20)
- zpl_adt_node returned by zpl_json_parse() could have more useful parents HOT 4
- zpl_opts module should make use of ADT parser to process input HOT 1
- ADT URL parser addition
- Off-by-one in zpl_json_write_string/zpl_csv_write_string_delimiter HOT 1
- Completion of error handling HOT 4
- zpl_array_fill ZPL_ASSERT error HOT 1
- zpl_opts_load_adt: allows the ability to load ADT node to fulfill CLI options
- ADT parsing of 'x' results in a ZPL_ADT_TYPE_INTEGER HOT 4
- `zpl_file_open` and functions that call it do not error when passed a directory HOT 5
- `name` and `string` fields of `zpl_json_object` have garbage values for `$schema` key HOT 6
- zpl_thread join/is_running
- Unicode support HOT 2
- pthread_join must be called for PTHREAD_CREATE_JOINABLE thread
- add 'zpl_semaphore_trywait'
- Undecorated 'cast' macro in helpers.h HOT 1
- Provide a way to free memory with the provided size HOT 5
- Global-buffer-overflow in zpl_strlcpyย ย in base64_enc HOT 2
- CSV parser assumes (unquoted) IP addresses are floating point HOT 3
- zpl_random_gen_isize / zpl_random_range_isize are not 32bit safe HOT 3
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
๐ Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. ๐๐๐
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google โค๏ธ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from zpl.