On 2022-Nov-14, Tom Lane wrote: > For discussion's sake, here's my current version of that 0004 patch, > rewritten to use list-of-bitmapset as the data structure.
I feel that there should be more commentary that explains what a multi-bms is. Not sure where, maybe just put it near the function that first appears in the file. > * The multi_bms prefix is a bit wordy, so I was thinking of shortening > the function names to mbms_xxx. Maybe that's too brief. I don't think the "ulti_" bytes add a lot, and short names are better. Either you know what a mbms is, or you don't. If the latter, then you jump to one of these functions in order to find out what the data structure is; after that, you can read the code and it should be clear enough. > * This is a pretty short list of functions so far. I'm not eager > to build out a bunch of dead code though. Is it OK to leave it > with just this much functionality until someone needs more? I agree with not adding dead code. > * I'm a little hesitant about whether the API actually should be > List-of-Bitmapset, or some dedicated struct as I had in the previous > version of 0004. This version is way less invasive in prepjointree.c > than that was, but the reason is there's ambiguity about what the > forced_null_vars Lists actually contain, which feels error-prone. Hmm ... if somebody makes a mistake, does the functionality break in obvious ways, or is it very hard to pinpoint what happened? > +/* > + * multi_bms_add_member > + * Add a new member to a list of bitmapsets. > + * > + * This is like bms_add_member, but for lists of bitmapsets. > + * The new member is identified by the zero-based index of the List > + * element it should go into, and the bit number to be set therein. > + */ > +List * > +multi_bms_add_member(List *mbms, int index1, int index2) Maybe s/index1/listidx/ or bitmapidx and s/index2/bitnr/ ? -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "La rebeldía es la virtud original del hombre" (Arthur Schopenhauer)
