On Fri, Sep 22, 2023 at 10:51:29AM +0200, Thomas Haller wrote: > On Thu, 2023-09-21 at 16:23 +0200, Pablo Neira Ayuso wrote: > > On Wed, Sep 20, 2023 at 09:23:46PM +0200, Thomas Haller wrote: > > > On Wed, 2023-09-20 at 20:10 +0200, Pablo Neira Ayuso wrote: > > > > On Wed, Sep 20, 2023 at 04:26:04PM +0200, Thomas Haller wrote: > > > > > Flags are not always bad. For example, as a function argument > > > > > they > > > > > allow > > > > > easier extension in the future. But with datatype's "flags" > > > > > argument and > > > > > enum datatype_flags there are no advantages of this approach. > > > > > > > > > > - replace DTYPE_F_PREFIX with a "bool f_prefix" field. This > > > > > could > > > > > even > > > > > be a bool:1 bitfield if we cared to represent the information > > > > > with > > > > > one bit only. For now it's not done because that would not > > > > > help > > > > > reducing > > > > > the size of the struct, so a bitfield is less preferable. > > > > > > > > > > - instead of DTYPE_F_ALLOC, use the refcnt of zero to represent > > > > > static > > > > > instances. Drop this redundant flag. > > > > > > > > Not sure I want to rely on refcnt to zero to identify dynamic > > > > datatypes. I think we need to consolidate datatype_set() to be > > > > used > > > > not only where this deals with dynamic datatypes, it might help > > > > improve traceability of datatype assignment. > > > > > > I don't understand. Could you elaborate about datatype_set()? > > > > I wonder if we could use datatype_set() to attach static datatypes > > too, instead of manually attaching datatypes, such as: > > > > expr->dtype = &integer_type; > > > > in case of future extensions, using consistently this helper function > > will help to identify datatype attachments. > > I think `expr->dtype = &integer_type` is fine, if > > - expr->dtype doesn't previously point to a datatype that requires > datatype_free() (e.g. because it's NULL). > > - the new datatype requires no datatype_get() (e.g. because it's > static). > > > > > > Btw, for dynamically allocated instances the refcnt is always > > > positive, > > > and for static ones it's always zero. The DTYPE_F_ALLOC flag is > > > redundant. > > > > That is a correct observation, but a (hipothetical) subtle bug in > > refcnt might lead to a dynamic datatype get to refcnt to zero, and > > that might be harder to track? > > > > Let me have a look if I can come up with some counter proposal to get > > rid of this flag, I would prefer not to infer the datatype class from > > reference counter value. > > If the reference counting is messed up, there is either a leak, a use- > after-free or modification of static data. I am refering to a hypothetical dynamic datatype object reaching refcnt == 0. > These are all bad bugs that needs fixing and are avoided by best > practices and testing. > > IMO keeping redundant state does not help with that or with > readability. > > I'd like to replace the "unsigned int flags" field with individual > boolean fields like "bool f_prefix" or "bool f_alloc". As for f_prefix, there is probably a way to infer this from context, I would need to have a look. > Dropping DTYPE_F_ALLOC/f_alloc flag altogether can be done (or not > done) independently from that.
