Simplify and remove some code from upb::SymbolTable #77
Conversation
This transitions it from shared ownership to unique ownership.
Also hid the dup() functions. We can't quite delete them yet because our current approach for extensions depends on duplicating defs.
| upb_status_seterrf(status, "Symtab already has a def named '%s'", | ||
| fullname); | ||
| goto err; | ||
| } | ||
| upb_def_donateref(def, ref_donor, s); | ||
| if (!upb_strtable_insert(&addtab, fullname, upb_value_ptr(def))) | ||
| goto oom_err; |
There was a problem hiding this comment.
in the error condition here it appears it won't undonate the ref of the current def
should you move the ref donation after adding successfully to addtab?
There was a problem hiding this comment.
Good call, done. One benefit of the direction we're going is that the symtab will have an allocator, and we can use that to inject malloc() failure into all these paths.
| @@ -2533,18 +2389,11 @@ static bool symtab_add(upb_symtab *s, upb_def *const*defs, size_t n, | |||
| oom_err: | |||
There was a problem hiding this comment.
line 2379 above appears unnecessary now since the symtab should not contain anything with the same name
| upb_def_donateref(def, ref_donor, s); | ||
| if (!upb_strtable_insert(&addtab, fullname, upb_value_ptr(def))) | ||
| goto oom_err; | ||
| def->came_from_user = true; |
There was a problem hiding this comment.
remove this member? it appears unused now
There was a problem hiding this comment.
It was there for the old way of adding extensions. But I just ripped that out too, because I don't think it was actually being used. So extension support is removed until the new, more proto2-like method is implemented.
This means extensions can't be used until we implement the replacement APIs for accessing extensions from a symtab.
| @@ -2533,18 +2389,11 @@ static bool symtab_add(upb_symtab *s, upb_def *const*defs, size_t n, | |||
| oom_err: | |||
This makes
upb::SymbolTableno longer refcounted. It also rips out the code to allow replacement of defs in a SymbolTable; this capability was not useful and was buggy. In the future we'll follow the model of the main protobuf implementation: messages can't be redefined.We do keep some of the legacy "dup" code around because it's currently used for extensions. However we will eventually want to remove this also and handle extensions in a different way.