Re: [lmi] [lmi-commits] master 783f4dd 5/9: Consolidate and revise documentation

[Vadim Zeitlin]

On Thu, 5 Apr 2018 18:33:50 +0000 Greg Chicares <address@hidden> wrote:

GC> I've heard others recommend describing
GC>  - what a function does, near its declaration; and
GC>  - how it does that, near its definition.

 This is broadly the way I do it, except that I don't have a single "how"
comment near the function definition, but multiple comments explaining the
non-obvious parts of the code inside the function itself, if necessary.
I.e. for me there is always one comment describing the function interface
from a public, external point of view and 0 to N comments inside its body
for things that are important internally but don't affect the public
contract.

GC> I don't like that guideline because:
GC> 
GC>  - I'm not convinced that what vs. how is a generally useful distinction.

 Maybe not if formulated like this, but I hope we can agree that the
distinction between public function contract and internal implementation
details is important.

GC>  - Every function would be documented twice, with different parts in 
different
GC> files. Flipping back and forth between two files is harder than focusing on 
one.

 FWIW I typically open a header and its corresponding source file in 2
vertical splits inside a single maximized Vim window, which allows to see
(or edit) both at once.

 I also don't think that there is any real issue with having multiple
comments. They serve different purposes and are used differently.

GC>  - I alter implementation more often than interface,

 Sorry, but this is the wrong comparison. The one which really counts here
is whether you modify the function more often than you use it. And, of
course, for a well-designed function, you should use it much more often
than you modify it, which is why it's much more important to be able to see
its comment easily -- meaning having it in the header.

GC>  - Comments in one file become outdated, and no longer reflect what's in the
GC> other file--or even grow to contradict it.

 If you change the function implementation to not match its public contract
any longer, you have problems much worse than mismatching comments.

GC>  - If code and commentary are both revised together in the implementation,
GC> then only the implementation need be recompiled. Header changes require more
GC> recompilation.

 The header comment needs to be changed only if the function interface
changed, which usually implies changes in the code using this function, and
hence recompiling it anyhow. So in most cases not much more code than
really needed would be recompiled and, in any case, is this really a
problem in practice? lmi is relatively small and even rebuilding it
entirely doesn't take that much time. At least I didn't notice it to ever
be a serious problem, did you?


GC> > Why should it be different for the functions?
GC> 
GC> Function comments often want to be long. Headers want to be terse, so that
GC> the whole thing can be gotten at a glance.

 Sorry, but this is unachievable. Average length of lmi header is 150
lines, which certainly doesn't fit on a screen and can't be overseen at a
glance. Longest header is 800 lines (granted, this is multidimgrid_any.hpp
which you didn't write, but any_member.hpp, which you did, is 700 lines)
and even the median is 106, so only a tiny minority (less than 10%) of lmi
headers can fit on a 26 line screen, even after discounting the 20 lines of
the boilerplate comment at the top.

 In addition to being unachievable, I don't see why would we strive to make
the headers terse -- at least any more so than any other file. Maybe this
would be a desirable consequence of other good practices, such as single
responsibility principle and general modularity, but why should this be a
goal on its own?

 But, again, pragmatically speaking, irrespectively of what the headers
want, they already don't get it.

GC> Then the header would be several times as long. No longer could I see all
GC> the public members on one single 26-line screen (given my myopia, that's all
GC> that fits when vim is maximized), and all the private members on another.

 Again, this is true, but this is already the case for plenty of classes
(it's less easy for me to count them than running "wc -l", so I can't
quantify it immediately, but there are definitely quite a few).

GC> Comparing all the declarations in either set would require moving my fingers
GC> rather than just my eyes. So that idea doesn't work.

 As above, I recommend opening (the same) file in 2 splits.

GC> That's why, as I remarked above, requiring a comment on every function's
GC> declaration so readily turns into a rule like "copy the implementation's
GC> first comment line into the header".

 This is a bad rule, duplicating the same comment, even partially, doesn't
make any sense, so let's not discuss it and just agree not to do this.

GC> /// This is used, e.g., when interest rates obtained from an external
GC> /// source vary from one year to the next, and it is desired to use
GC> /// them as lmi input. It might seem that...
GC> 
GC> Which question does that answer: what, or how? Neither. It answers
GC> why. So where does it belong: header, or implementation?

 I don't know this code well enough to answer this definitely, but the
criterion is simple: if it affects the function caller, then it must be in
the header. If the comment is only important to the function implementor,
then it must be in the code. It's really a very simple rule and in practice
I can't remember ever having any problem with deciding whether the given
comment should go into the header or the implementation.

GC> >  I really wish this rule could be changed and the documentation comments
GC> > could appear at the point of function declaration instead. It would
GC> > definitely make reading them much more convenient for me personally and I
GC> > just don't see any drawback of keeping them there.
GC> 
GC> I hope I've explained the drawbacks, which I regard as fatal.

 I'm not sure which one of them is fatal. Surely at least some of them are
too minor to have any importance? So is it the combination of all of them
which results in this? If so, would removing those that are based on the
drawbacks of keeping the same comment in 2 places, which is obviously bad
and shouldn't be done, change anything?

 
GC> >  What am I missing? Why does lmi do it differently from all the other C++
GC> > (and even C) headers in existence?
GC> Surely that's a bit hyperbolic.

 I'm afraid it isn't. I must have looked at hundreds if not thousands of
C++ libraries and I can't remember any of them except, indeed, some Boost
libraries, which had comments explaining the function interface in the
source files and not in the headers (admittedly, quite a few of them didn't
have any [useful] comments at all, which is not better, but still less
surprising).

GC> Look at page 539 of the C++17 standard

 Are we really going to pretend that C++ standard shows the optimal coding
style? Surely it's well understood not to be the case, otherwise we would
all be using single letter variable names only everywhere.

GC> (N4659.pdf), which is [pairs.pair]. This is header perfection. All three
GC> "template<class U1, class U2>" ctors appear on three successive lines.

 This is because there is no documentation at all. Or, rather,
documentation is external and provided by compiler vendors or C++ reference
site or whatever. It's not a bad approach, if you can afford it, but you
typically can't because maintaining documentation outside of the code is
just too difficult and time-consuming, especially when your code base
changes more often than the ISO standard.

GC> Or look at class crc_basic here:
GC>   https://www.boost.org/doc/libs/1_66_0/boost/crc.hpp

 Yes, Boost is indeed exceptional, presumably because it mimics the
standard headers as its libraries are supposed to be used as a pattern for
standardization. And it also does maintain its documentation externally, in
a different format (and it also uses bjam, so let's not pretend that
everything Boost does is a good idea). Notice that Boost.CRC doesn't have
any comments in its source files neither (because it doesn't have any), so
this is an example of a library mentioned above, which just doesn't have
any interface comments at all. Is this really a good example to follow?


 Anyhow, to summarize: I agree that having a synopsis of a class can be
useful. I strongly disagree with the fact that having it is more useful
than having comments describing the class methods public interface and
IMO the lack of such comments in the header strongly harms lmi code
readability and significantly hinders writing new code using the existing
functions. And I don't believe there is any realistic possibility of
maintaining interface-level documentation externally, so continuing to ban
comments in the header means that such documentation won't exist, at least
in easily accessible state, at all which is, again, a real problem IMO.

 Can we find some compromise that would allow to address this problem or do
you think that this is not a problem at all or a problem not worth
addressing? To be honest, I don't have any completely satisfactory
suggestions, but I do have one which might be acceptable to you and would
improve things significantly, even if it's not perfect: what if we extended
the class comment -- which doesn't seem to be target to any constraints on
its length for some reason -- to describe the interface/contracts of its
member functions? And/or could the rule about not having long comments in
the header be relaxed for global functions, as they're not subject to "keep
all class synopsis on one screen" constraint, being not inside any class
anyhow?

 To be maximally concrete, this discussion began due to the changes in the
commit 783f4dd835466ada15c6ff38c934ae605639f26a which removed the comment

        // Dtor checks if save() had been called, so don't forget to do it.

from pdf_writer_wx class header and I started it because I think this
comment is very important and should absolutely be seen by all users of
this class. If you're adamantly against having it in the class body, it
wouldn't be much worse (although still worse, IMHO, because it would put it
further away from the function it pertains to) to put it in front of the
class declaration. I.e. we could move all the comments from inside of the
class outside/before it. I didn't do it like this initially because I think
it's much simpler to read one short comment directly related to the
function you're looking at, than find the relevant part of the long comment
before the class description pertaining to it, but at least having the
comment in the same file is better than having it in a completely different
one.

 So should we do it like this? Of course, this wouldn't change the total
number of lines in the header at all, so the only drawback listed above
that this could help with would be the one about seeing all the public
methods of the class at a single screen, which seems like a pretty minor
improvement, but OTOH we do have plenty of long comments before the class
declaration, so they don't seem to be a problem.

 Can I please have my comments in the header in this form, i.e. documenting
only the interface (and not the implementation) and only before the class
or global functions declarations?

 Thanks,
VZ