Hi Eli, Thanks for the review and explanation, I addressed the issues you brought up and it should be ready for the next round. > If you edit with Emacs, then the .dir-locals.el file in our repository > should set up the indentation for you, and then marking the region and > typing C-M-\ will reindent the region according to our rules. Somehow my Emacs does not respect the .dir-locals.el file, but I used clang-format to format the changed lines > Also, our style is not to use braces when the block has only one > statement. I have to say I had a hard time with this :P I should have removed the braces from the one statement blocks. > About "enough space" -- how many features could a font have? And what > to do if it has more than 32? GNU coding conventions frown on > arbitrary limits, and 32 sounds like it's quite arbitrary. Can we do > better? I changed it to dynamically increase the size using existing xpalloc pattern > + Lisp_Object feature_sym = XCAR (feature_spec); > + Lisp_Object feature_val = XCDR (feature_spec); > + > + if (SYMBOLP (feature_sym) && FIXNUMP (feature_val)) > > Are we sure the value cannot be larger than most-positive-fixnum? > Emacs can handle larger values if needed. According to the harfbuzz document and the fonts I've seen, mostly the values are either 0 or 1, occasionally you see 3 for different variations. This should be fine. > As mentioned above, I think we should be smarter here: instead of > overwriting the value of :font-features of the target face, we should > merge the two lists so that it includes the features from both faces. > Font features from FROM should override the same features from TO, but > font features which exist in TO but not in FROM should be left in the > resulting list. I added support for attribute merging according to the review comment. > In addition, this will need s NEWS item and a suitable addition to the > ELisp manual, where the face attributes are described. That's right, I've added both. > Last, but not least, to accept a contribution of this size, we will > need you to sign a copyright-assignment agreement with the FSF. If > you are prepared to do that, I will send you the form to fill and the > instructions to go with it. I have signed and sent the scanned version and I am currently waiting for the digital copy from FSF.