On Fri 05 Mar 2021 at 17:51:39 -0600, Matthew D. Fuller wrote: > 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?
Exactly. I think I noticed because I got a weird double message. > - 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? I did check edge cases with removing all windows from the ring one by one and adding some back. It turned out that the Occupy Window is also on the ring so it isn't really empty yet when you would think it is. I suspect the window ring used to be a normal doubly-linked list at some time in the past. That would explain the existing checks for NULL. With the factored out adding and removing code, there is also more assurance that NULLs cannot sneak in. > - r681: WindowIsOnRing() should be static? If it stays there, yes, but (see next item) > 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? Yes, I wasn't really sure where exactly to put them. functions_internal.h is a bit of a strange header file since it contains declarations for functions in multiple functions_*.c files, including functions_warp.c, which by itself seems like a nice place for window (warp) ring related stuff. But I don't like the inclusion of functions_internal.h much either. Maybe it's worth some new files, win_ring.[ch]. It would be at least as meaningful as clicktofocus.c. I'm adding a revision with this change. -Olaf. -- ___ Q: "What's an anagram of Banach-Tarski?" -- Olaf "Rhialto" Seibert \X/ A: "Banach-Tarski Banach-Tarski." -- rhialto at falu dot nl
signature.asc
Description: PGP signature
