On 15/05/2020 13.28, Eli Zaretskii wrote: >> Cc: 41200@debbugs.gnu.org, Anders Lindgren >> From: Clément Pit-Claudel >> Date: Fri, 15 May 2020 12:22:53 -0400 >> >>>>> face_hash_table? >>>> >>>> Thanks, good idea. I also liked Stefan's frame_face_map. >>> >>> "Map" is too general, IMO, it doesn't tell enough about the kind of >>> object it is. >> >> Got it. Is face_table better? (that was Stefan's other suggestion) > > Is anything wrong with face_hash_table? Nope; I've attached a new patch. >> Btw, I have one more question: some callers of face-list seems to rely on the order of faces added to it, so it would be better to preserve that order. Since hash-tables are not necessarily ordered, should I sort faces by face-id before returning them? Done in the attached patch as well. >>> I think the problem is that tab-line is declared a basic face, but its >>> defface form is not in faces.el. >> >> Ah, good catch. Current there's a defface for tab-bar in lisp/tab-bar, and since that's preloaded it works, but the defface for tab-line is in lisp/tab-line.el and so isn't preloaded. >> Should I move both to faces.el? >> > Yes, I think so. Thanks. I will ask Juri to confirm before moving them, because I realize now that they have a custom group. Juri (CC'd; hi Juri!), was there a reason to make tab-bar and tab-line basic faces? I see they are both in their own files and groups, instead of being in faces.el. >>>>> I'm not sure I understand why do you need to look at the existing >>>>> face's 'face' property? The original code didn't. >>>> >>>> The original code iterated over face-frame-alist in the order in which entries were added to it. If I understand correctly, iteration order isn't guaranteed on hash tables (is it?), so I had to find a different source of truth for these. >>> >>> Maybe we should store the ID with the face? I think we wanted to get >>> rid of the 'face' property of the faces at some point. >> >> Sounds reasonable; but where would we store it? Right now faces are just symbols, right? > > Don't hash-tables allow us to store more than one item in each slot? They do, so I can certainly store (face-id . face-vector) in the hash table. Done in the attached patch. I only do this in Vface_new_frame_defaults, not in frame->face_hash_table. >>> No, I was asking why not start with it as nil and actually make a >>> hash-table when we first need it. >> >> Oh, I thought it would be simpler to always have a hash table instead of having to sanity check every time. > > What is the default size of a hash-table? 65 entries, I think.