-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Maintenance swipe: get rid of some global variables in favour of actual data flow visibility #82
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Jan Pokorný <[email protected]>
This is a primitive that's easier to follow/more terse. Looking forward, we skip some places where we want to introduce a refactoring to drop an undesired non-transparent reliance on booth_conf global variable prior to modifying these two macros to that effect as well (respective places, at least, receive white-space-around-operators sanitization). Signed-off-by: Jan Pokorný <[email protected]>
This is to allow for better visual identification of the macro magic. Signed-off-by: Jan Pokorný <[email protected]>
And not to forget ... current passing of the whole I haven't delved into that business yet, but it's one |
Wrt. fine-graining, desired access method (read-only/ok-to-modify) for |
So it imposes structural (vs. hazy flat) patterns on multiple levels, |
Also, apply the same recursively to both the underlying and superjacent call graph expansions (respective static functions). Signed-off-by: Jan Pokorný <[email protected]>
Signed-off-by: Jan Pokorný <[email protected]>
Also, apply the same recursively to both the underlying and superjacent call graph expansion (respective static functions for the former, progression up to main.c, i.e. do_attr_command, for the latter). Also, exercise some const-correctness at places being touched, and mark unused function as such. Signed-off-by: Jan Pokorný <[email protected]>
Also, apply the same recursively to both the underlying and superjacent call graph expansion (respective static functions for the former, progression up to main.c, i.e. setup_ticket, for the latter). Signed-off-by: Jan Pokorný <[email protected]>
Some previous commits amended, some new added. Known issue:
This is because of intentionally crafted asserts to discover exactly Need to look into the call chain whether it's not malign, to begin with. |
Also, apply the same recursively to both the underlying and superjacent call graph expansion (respective static functions[*] for the former, progression up to main.c). [*] _find_myself gets marked static as it should have been as of 56801bd Signed-off-by: Jan Pokorný <[email protected]>
There's no real need since it's already carried in the "local" var[*] carrying site data for what's resolved as a local site. Since it's not a straightforward expression to pick up the port stored inside SITE->[union: either sa4 or sa6 member], another inline getter moreover not crashing on SITE being NULL was devised: site_port. That helper is consequently applied also at other places that deal with fetching port number -- antithetically, they used to fetch from said booth_conf global variable while, at the same time, other connection relevant information used to be fetched from whole another memory object (said "local site" global). In effect, even more undesired dependencies on booth_conf global were removed, in favour of unifying the source of data at said "local site" one. Also, insist on const-correctness with the sibling of added inline function (so they look uniformly, the new one also follows that rule). [*] also global but that's not of interest for the time being Signed-off-by: Jan Pokorný <[email protected]>
Also, apply the same recursively to superjacent call graph expansion (progression up to main.c), and apply that whenever it is not eligible where it previously was not (because FOREACH_{NODE,TICKET} not accepting struct booth_config * injection. Also, exercise some const-correctness at places being touched, and hide some functions as merely static ones. Signed-off-by: Jan Pokorný <[email protected]>
Also, apply the same recursively to superjacent call graph expansion. Also, exercise some const-correctness at places being touched, and hide some functions as merely static ones. Signed-off-by: Jan Pokorný <[email protected]>
Mark function being effectively static (and move around the file so as not to require an extra declaration) to that effect. Signed-off-by: Jan Pokorný <[email protected]>
Consequently, foreach_tkt_req (what said function is ultimately passed into) that practically implements high order functional do-for-all pattern with the same subset of arguments that itself receives, needs to be extended with this extra argument as well. Also, apply the same recursively to superjacent call graph expansion (progression up to main.c). Also mark function being effectively static. Signed-off-by: Jan Pokorný <[email protected]>
Signed-off-by: Jan Pokorný <[email protected]>
Signed-off-by: Jan Pokorný <[email protected]>
The only exception, transport(), is kept since it is intertwined with another global, hence dealing with it is postponed for now. Signed-off-by: Jan Pokorný <[email protected]>
And except for said single use intermingled with another global mentioned in the previous commit: transport() of inline-fn.h. Signed-off-by: Jan Pokorný <[email protected]>
Even then, booth_conf global variable cannot be turned into static one, since there is this multi-global complication that was mentioned already: transport() of inline-fn.h. Signed-off-by: Jan Pokorný <[email protected]>
Finish ~ finally mark static. As for booth_transport, it is still more covenient to split definition and actual use across two files (transport.c and main.c, respectively), so we at least make the semi-hidden nature (no presence in any of the actual header files) apparent with the double-underscore after initial namespace prefix convenience. Also fix a latent "no extern storage class specifier specified" in case of compiling with -fno-common/default since GCC 10 (for missing "extern" qualifier). Signed-off-by: Jan Pokorný <[email protected]>
It is still more covenient to split definition and actual use across two files (pacemaker.c and main.c, respectively), so we at least make the semi-hidden nature (no presence in any of the actual header files) apparent with the double-underscore after initial namespace prefix convenience. Also fix a latent "no extern storage class specifier specified" in case of compiling with -fno-common/default since GCC 10 (for missing "extern" qualifier). Signed-off-by: Jan Pokorný <[email protected]>
At that occasion, make "struct booth_config" contain "path_to_self" as it is handy to have it there and not to rely on presence of the former struct. Signed-off-by: Jan Pokorný <[email protected]>
Signed-off-by: Jan Pokorný <[email protected]>
Thanks to the change with the former, attaching it to the now de facto generalized "context" in struct booth_config being passed around, we can now simplify find_myself (both underscore-prefixed and plain) so that no extra outbound argument propagation is needed (argument is dropped). Signed-off-by: Jan Pokorný <[email protected]>
Signed-off-by: Jan Pokorný <[email protected]>
This module is supposed to hold generic functionality satisfying no-binding-but-libc. Signed-off-by: Jan Pokorný <[email protected]>
Also, type the argument properly. Signed-off-by: Jan Pokorný <[email protected]>
Signed-off-by: Jan Pokorný <[email protected]>
Signed-off-by: Jan Pokorný <[email protected]>
It is indeed less than optimal for the booth_config to be shared as is. Thanks for bringing this up. |
Hi Jan, apparently you are not involved any more with HA. Wanted to say that it is a pity that we don't have your help anymore, but I guess that it was not easy fighting against proverbial wind mills. It is my fault that I often did not have time to work on your pull request, though heaven knows that I really tried. Occasionally, it would be just too much for me. But that doesn't mean that your PRs were bad, to the contrary, they were always of excellent quality and your engagement was always very much appreciated. |
Having said all that, I hope that you are doing fine, wherever you work now. Cheers, Dejan (-: |
Don't mind me, just making a checklist so I know which patches I've merged and which I still have left to do.
|
This is a first step in a direction towards less rigid/cumbersome
code, easing the understandability and maintenance as such.
Get rid of (some, commonly shared) global variables means:
less hidden, behind the curtains state, meaning less
of surprising side-effects, better understandable data
flow, effectively containing the impact radius of the
functions
unit tests are more feasible (easier to conduct) when the
inputs to the computation/functions are not part of said
non-apparent global state, but rather part of the explicit
input to the them
likewise, any "scope of authority" etc. types of regrouping
of the functions in modules is not easily done when the
data flow is partially obfuscated
more dynamic handling is enabled (e.g. multithreading or
fetching a new instance of the configuration, which can then
be conditionally promoted to the life configuration)
if there's a good need for that
last but not least, using global-scoped variables is not
considered a praised practice
For
struct booth_config
in particular, it was dynamicallyalloc'd at heap, so there was no gain in terms of avoiding
relevant class of bugs.
Also, this PR reconciles the no-code-doc problem at the functions
being touched throughout. And this swipe helped to identify
both unused and needlessly exported functions.