Refactor: header guard + drop some dead end references #1224
Conversation
Originating in discussion around commit ef711f9, there's a Ken's concern about not enough uniqueness in artificial header inclusion guard names (in that particular case) on the table. Together with varying styles used for these names across the project and the guards sometimes missing altogether, this is to be addressed in two subsequent commits (adjust existing + fix missing ones). With this first step, we further introduce following naming scheme: 1. all guards start with PCMK ands end with __H 2. there are 5 purposeful patterns so that the guard name also encodes some not-so-obvious pieces of information: PCMKDEF_<HEADER>__H: shared headers being a mere dictionary assigning symbolic names to constants/literals, no type/function/object business here PCMKLIB_<LIBRARY-or-MIX>_<HEADER>__H: public headers for a library (or a mix of libraries designated with "MIX") PCMKPRIV_<LIBRARY-or-MIX>_<HEADER>__H: private headers for a library (or a mix of libraries designated with "MIX") created more-or-less in an organic way, function promises via "extern" and not via proper declarations, etc. PCMKINT_<LIBRARY-or-MIX>_<HEADER>__H: private headers for a library (or a mix of libraries designated with "MIX") created in more decent way, declaring functions properly (with hidden visibility, cf. G_GNUC_INTERNAL from glib) hence usable in all the contexts of standard library-internal use PCMK_<DIR-or-BINARY>_<HEADER>__H: private headers exclusively for compiling particular binary Some kinds of normalization is also applied on lines around respective preprocessor conditionals and comments. (As an aside, there used to be unintendedly mixed "ElECTION" in a guard name prior to this change.)
Appeared in 83919a1, slighty modified on serveral occassions, but still pretty blind.
|
The naming scheme is way too formalized and complex; it's neither intuitive to someone reading the code, nor easily maintainable in a large, distributed project. We need a much more free-form approach with as few rules as possible. A good choice would be prefixing all symbols with "PCMK_" and ending with "__H", and otherwise a free-form similarity to the filename. The only goal should be to prevent re-inclusion of the same file, not encode any information. |
|
On 13/02/17 08:33 -0800, Ken Gaillot wrote:
The naming scheme is way too formalized and complex; it's neither
intuitive to someone reading the code,
...who usually has no business in understanding what the particular
pattern was meant to encode, it's just an inclusion guard in this view
nor easily maintainable in a large, distributed project.
Well, it's ok if the naming scheme corrodes a bit in time, but there
should be some extra distinguishing marks to get an orientation, say,
in what's public and what's private API. So far, pacemaker seems quite
fuzzy in this regard, hence establishing (in part estimated)
directions here may help in future steps towards clarity.
That being said, I did the heavy lifting already, should be easy to
keep an eye on the header changes from now on.
We need a much more free-form approach with as few rules as
possible. A good choice would be prefixing all symbols with "PCMK_"
and ending with "__H", and otherwise a free-form similarity to the
filename.
Applied guard names already conform to that, so there's not conflict.
The only goal should be to prevent re-inclusion of the
same file, not encode any information.
Considering the amount of header files, it definitely helps to
have some system to get some orientation assistance (mass-grep'ing
etc.) for those who would make a use of it (counting myself in),
of course others can take it just as JOHNDOE placeholders, that
were at least made less likely to collide with the surrounding
system.
In other words, I don't understand what complexity you are arguing
against if it is so easy to ignore the actual patterns behind the
now-unique-enough names. We don't expect the number of header files
to double any time soon, are we?
|
|
On 13/02/17 18:50 +0100, Jan Pokorný wrote:
On 13/02/17 08:33 -0800, Ken Gaillot wrote:
> The naming scheme is way too formalized and complex; it's neither
> intuitive to someone reading the code,
...who usually has no business in understanding what the particular
pattern was meant to encode, it's just an inclusion guard in this view
> nor easily maintainable in a large, distributed project.
Well, it's ok if the naming scheme corrodes a bit in time, but there
should be some extra distinguishing marks to get an orientation, say,
in what's public and what's private API. So far, pacemaker seems quite
fuzzy in this regard, hence establishing (in part estimated)
directions here may help in future steps towards clarity.
Actually, I also see an immediate benefit in having a header file
associated with a specific library in this way. Another thing
pacemaker codebase is not entirely clear about at times.
…--
Jan (Poki)
|
It's very important in a distributed, community-driven project that code be as readable as possible to a newcomer, without mysterious coded parts that need a separate guide to decipher. Pacemaker code is already very difficult to follow in many cases due to its asynchronous, event-driven nature and multiple daemons -- we need to lower the barrier to understanding, as much as possible.
The names picked here will be fine if you get rid of the extra part between PCMK and underbar, so that your #1 is the only hard rule.
Most developers won't (and shouldn't) simply ignore parts of the code they don't understand in the area they're working on. If something's meaning is obvious, they can move on to the part they're interested in, but if it's mysterious, they need to waste time figuring out why it's the way it is before they can feel confident enough to change anything near it. |
|
On 13/02/17 11:20 -0800, Ken Gaillot wrote:
> In other words, I don't understand what complexity you are arguing
> against if it is so easy to ignore the actual patterns behind the
> now-unique-enough names.
Most developers won't (and shouldn't) simply ignore parts of the
code they don't understand in the area they're working on. If
something's meaning is obvious, they can move on to the part they're
interested in, but if it's mysterious, they need to waste time
figuring out why it's the way it is before they can feel confident
enough to change anything near it.
They hardly need to spend time on identifying a mere inclusion guard,
something such customary (they should get some C classes otherwise).
Look, I clearly look at the extra information added as something that
developers can actually benefit from if they are into these gory
details -- I am certain I will in the future.
But now we are falling into plain bikeshedding: you are afraid someone
could be confused. Well, "git blame" will unveil the mystery of the
patterns in the order of tens of seconds, should anyone be so curious.
…--
Jan (Poki)
|
|
Someone should be able to add a new header file by copying and editing an existing one without needing a decoding guide. Additionally, without any enforcing mechanism, this is guaranteed to become wrong over time, at which point it becomes unreliable and can cause more trouble than it helps. If we want to describe the purpose of a file, that's best done in a doxygen file block. |
|
On 13/02/17 12:36 -0800, Ken Gaillot wrote:
Someone should be able to add a new header file by copying and
editing an existing one without needing a decoding guide.
At worst it ends up as PCMK_foo__H. No issue here.
Additionally, without any enforcing mechanism, this is guaranteed to
become wrong over time, at which point it becomes unreliable and can
cause more trouble than it helps.
Unreliable only for the users of the scheme, which will want to fix
it anyway. No issue here++.
If we want to describe the purpose of a file, that's best done in a
doxygen file block.
Yes, too many things far far from ideal, though.
…--
Jan (Poki)
|
Right, if you redo this as PCMK_foo_H, I'll merge it. Header guards should simply be unique; they should not encode any information. Readability and maintainability are more important here. |
|
On 13/02/17 16:31 -0800, Ken Gaillot wrote:
> At worst it ends up as PCMK_foo__H. No issue here.
Right, if you redo this as PCMK_foo_H, I'll merge it.
Header guards should simply be unique; they should not encode any
information. Readability and maintainability are more important
here.
You don't understand. Currently, the header files are a mess.
When we get into the point no extra guides like the suggested ones are
needed, it will indeed be natural to no longer carry them.
I think I've already spent enough of time of defending what I think
they are handy _now_. The change can happen anytime just using
something like this:
find -name '*.h' -print0 \
| xargs -0 sed -i 's|\( PCMK\)\(DEF\|[^_ ]\+_[^_ ]\+\)_\([^_ ]*__H\)|\1\3|'
Given the easiness of such an extra info losing step (and I would
strongly prefer to retain that now, it was the whole point), can we
please take it in as is, unless of course you have further complaints,
please?
Dropping well-formed data is easy, in steep contrast with actually
creating them in a batch, which is what has already been done here.
…--
Jan (Poki)
|
If you do that and update the first commit message, I'll merge this. I am not merging a complex encoding of information into header guards. That is not the appropriate place for such information. A large, distributed, community-driven project such as pacemaker must favor readability and maintainability across a wide range of developer mindsets and styles. Trust me, encoding information into header guards does not have wide appeal among developers. It hinders readability and maintainability. Our goal here is to help solve users' problems, not create a crystalline structure of pristine purity. |
|
Ah, but the maintainability for the time being is the exact goal for that scheme... |
|
Let's not let this request gather dust. Please make the requested change, and I'll merge it. |
|
Please make sure you understand the situation. One more thing you should realize: if that command is applied (well, Last one time: I don't agree with your proposed (bikeshed-level in my That being said, I am checking "allow edits from maintainers" box; (And I will be free to undo that change again for me locally via local |
|
(not endorse ... at this time) |
|
Cherry-picked the "Cleanup" commits, and merged with #1229 If you are able to make the requested change, feel free to re-submit the others. |
|
Ken's first comment is spot on as demonstrated by the first "new" guard (PCMK_ATTRD_INTERNAL__H) which ironically doesn't conform to the proposed naming scheme. |
My eyes need rest now.