Hi Eli, Thanks a lot for the review. I have attached an updated version of the patch. On 15/05/2020 07.05, Eli Zaretskii wrote: > I'd like to keep the old face-new-frame-defaults and frame-face-alist > for compatibility, but mention in the doc strings that they no longer > return modifiable values, and perhaps deprecate them. Done for frame-face-alist. But how can one do a read-only variable? Using the new variable watchers facility? >> * The name face_hash isn't ideal, since there's already a distinct notion of face hashes (hash codes). Can you think of a better name? > > face_hash_table? Thanks, good idea. I also liked Stefan's frame_face_map. >> + // QUESTION: is this where this should be initialized? > > Yes, I think so. But do we need to do anything when frame is deleted > as well? I'm not sure (I don't know how garbage collection works on the C side in Elisp; I assumed the map would be collected). >> -DEFUN ("frame-face-alist", Fframe_face_alist, Sframe_face_alist, >> +// QUESTION: Should I add an ELisp version of frame-face-hash? > > You mean, frame-face-alist, right? Yes, most definitely: I imagine a > lot of code out there uses that, and we wouldn't want to break that. Done. I looked around, but I didn't find many uses at all (for example, there are none in ELPA). I think this is likely because the function is documented as "For internal use only." There are no uses of face-new-frame-defaults in ELPA either; online, I found many copies of lisp-mode and emacs-lisp-mode, which refer it, and a few functions derived from edebug-eval-defun, which references it. > And I'm not sure we should have it only in Lisp: perhaps we should > maintain the alist as well, and add/remove to/from it when a face is > added or removed in the hash table. Otherwise this change of > internals will have painful effect on packages that use the current > APIs. frame-face-alist is likely less crucial than face-new-frame-defaults, because it was already a function, so the return value has to be mutated in place to modify it (it couldn't be directly assigned). For both of these, however, how would we ensure that the alist remains in sync with the hashmap (that is, how do we catch modifications?) >> + Lisp_Object lface = HASH_KEY(table, idx); >> + Lisp_Object face_id = Fget (lface, Qface); >> + // FIXME why is (get 'tab-line 'face) 0? > > A bug, I guess. As far as I can see, these IDs are assigned by Finternal_make_lisp_face, and I *think* it is never called for tab-line? Should I make sure that it is? If so, where from? > >> + if (!FIXNATP (face_id)) >> + // FIXME: I'm not sure what to do in this case > > 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. > >> DEFVAR_LISP ("face-new-frame-defaults", Vface_new_frame_defaults, >> doc: /* List of global face definitions (for internal use only.) */); >> - Vface_new_frame_defaults = Qnil; >> + Vface_new_frame_defaults = >> + make_hash_table (hashtest_eq, DEFAULT_HASH_SIZE, DEFAULT_REHASH_SIZE, >> + DEFAULT_REHASH_THRESHOLD, Qnil, Qnil); > > Why do we need to start with a non-default hash-table? I wanted to use `eq' instead of `eql' as the test (is that what you were asking?)