On Fri, Mar 05, 2021 at 02:09:08PM +0100 I heard the voice of Rhialto, and lo! it spake thus: > > They are in my "small beer" branch: lp:~rhialto/ctwm/smallbeer > > If there are no objections I will merge these to trunk in a few > days.
Mostly LGTM. I don't have any deep knowledge or strong feelings about the touched code, so it's a fairly surface review. And all the tests still pass, so it must be flawless, right? :> Couple random notes and one slightly bigger: - r676, "Add a missing break statement." By my reading, this doesn't _functionally_ change anything (except maybe a doubled up weird error message if you mess up your config somehow), just makes the code flow more proper? - The NULL check removals in the ring manglement funcs. Sounds reasonably in general, but wanted to be sure you checked it in edge cases; e.g., if there's nothing in the ring or the like, might we have NULL's snuck in somehow? - r681: WindowIsOnRing() should be static? And the one slightly bigger: - AddWindowToRing() and friends should probably live somewhere other than functions_internal.c, since they're used from code not related to functions. Pulling in functions_internal.h in non function*.c files smells a bit. Maybe win_utils.[ch]? 's a bit of a dumping ground, but maybe it's the right dumping ground? -- Matthew Fuller (MF4839) | fulle...@over-yonder.net Systems/Network Administrator | http://www.over-yonder.net/~fullermd/ On the Internet, nobody can hear you scream.
