RFC: Largish Cleanup and Refactoring #1191
Conversation
|
(The failing checks are an issue with the tests on ubuntu. GCC or GLIBC changed something and now the fortified headers do not let you use strncpy to copy an exact number of bytes without room for a NUL terminator. I suspect it can be fixed by replacing strncpy with memcpy but I can't really test because its not broken on my system.) |
There was a problem hiding this comment.
Looks good to me.
Personally, I do not mind both versions.
I think both variants are very readable and the compiler should inline most of the get-/ set-methods anyway.
Not sure though, what I think about moving validity checks from some set-methods to their call sides.
There is the benefit that the code is more explicit, but the drawback is that the checks are easier missed in future code.
| int col = view_cursors_cell_get(cur); | ||
| if (!line || col == -1) | ||
| Line *line = cur->line; | ||
| if (!line) |
There was a problem hiding this comment.
Is this not missing the original check col == -1?
There was a problem hiding this comment.
Nope, the col == -1 was only there because view_cursors_cell_get() would return -1 when cur->line == NULL. It was just hidden behind the redirection.
There was a problem hiding this comment.
I see. So the check was redundant anyway.
Sorry I missed that view_cursors_cell_get returned -1 previously.
Not really, since they are defined in different translation units. Hence the 10k reduction in binary size.
I think most of things I removed were not doing that. The ones that were checking for validity were also checking for validity at their call site so the work was just being doubled for no reason. That is the primary benefit of being explicit about what you are doing rather than calling some function and ignoring the internals of the function (as you should). |
rnpnr
force-pushed
the
cleanup
branch
2 times, most recently
from
36ebc28 to
18aa569
Compare
This removes the function pointer interface which was adding needless complexity and making it difficult to add new events. Now if new events are only meant for lua they only need to be added to the lua interface. This will also have a minor reduction in runtime memory usage and produce a smaller binary. The only runtime difference is that QUIT happens after all windows have been closed and their files freed.
There only exists a single Ui so there is no need to force a pointer redirection for accessing it. The Ui member was moved down in vis-core.h to punt around an issue with the way lua checks for existing objects. It may show up again as I flatten more structs.
Same as previous commit each window only has a single View. No need for it to be stored elsewhere in memory.
I was growing tired of trying to debug things in vis when everything had to jump through many levels of redirection because all of the core structures were opaque. So I went through and cleaned all that up. While I was at it I shuffled some things around so that now when you want to add a new event to lua you don't need to define a stub function for when lua is disabled.
The only functional change that should have happened is that the QUIT event now happens after all windows have been closed and their files freed. Everything else should be the same as before.
Obviously a 450 line reduction in code is nice and on my system the binary size was also reduced by ~10kB. At runtime there is probably a minor performance benefit and lower memory usage but I didn't really measure those.
I'm still not a fan of how all the header files are interdependent and tangled up but fixing that will have to wait until later.