-
Notifications
You must be signed in to change notification settings - Fork 262
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Split upb::Arena/upb::Allocator from upb::Environment. #58
Conversation
This will allow arenas and allocators to be used independently of environments, which will be important for an upcoming change (a message representation). Overall this design feels cleaner that the previous Environment/SeededAllocator design. As part of this change, moved all allocations in upb to use a global allocator instead of hard-coding malloc/free. This will allow injecting OOM faults for more robust testing. One place that doesn't use the global allocator is the tracked ref code. Instead of its previous approach of CHECK_OOM() after every malloc() or table insert, it simply uses an allocator that does this automatically. I moved Allocator/Arena/Environment into upb.h. This seems principled since these are the only types in upb whose size is directly exposed to users, since they form the basis of memory allocation strategy.
Review to @scwhittle. I think you're going to like this! |
Tests are green now. |
free((void*)def->fullname); | ||
def->fullname = upb_strdup(fullname); | ||
if (!upb_isident(fullname, strlen(fullname), true, s)) { | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
errmsg?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
upb_isident() takes care of that.
Addressed comments, PTAL. |
/* Need better logic here; at this point we've qualified some names but | ||
* not others. */ | ||
return false; | ||
} | ||
upb_def_setfullname(def, name, NULL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this may fail with oom too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are probably lots of places like this, and we don't have a great recovery story here. I'm going to punt on being super thorough with this until we're actually testing for it anyway by injecting malloc failures.
LGTM |
Thanks! Merging. |
This will allow arenas and allocators to be used
independently of environments, which will be important
for an upcoming change (a message representation).
Overall this design feels cleaner that the previous
Environment/SeededAllocator design.
As part of this change, moved all allocations in upb
to use a global allocator instead of hard-coding
malloc/free. This will allow injecting OOM faults
for more robust testing.
One place that doesn't use the global allocator is
the tracked ref code. Instead of its previous approach
of CHECK_OOM() after every malloc() or table insert, it
simply uses an allocator that does this automatically.
I moved Allocator/Arena/Environment into upb.h.
This seems principled since these are the only types
in upb whose size is directly exposed to users, since
they form the basis of memory allocation strategy.