Re: [Libcdio-devel] New git branch for bug 45017 ? Or re-use branch for 45015 ?

[Rocky Bernstein]

Comments in line.

On Mon, Jul 31, 2017 at 3:02 AM, Thomas Schmitt <address@hidden> wrote:

> Hi,
>
> i am not so happy with the comment change you made for
> iso9660_check_dir_block_end().
>

I am sorry. Let's git this fixed.


>
> Rocky Bernstein changed:
> > - [...] The caller should
> > - then skip the next actions in the loop and rather hop to the loop start
> > - by "continue".
> > - If "false" is returned, then processing of the caller's loop shall go
> on
> > - normally.
> > + [...] The caller often skips actions only when at the end of a
> > + record list.
>
> There is no "often" reaction but "always" the loop continuation with all
> five callers. The prescription to do the "continue" is taken directly
> from the five occasions where the new function calls replaced the original
> code.
>

Ok. But the comment should represent what's going on in *that* code not
the code that calls it. Right now it may be the case that it is embedded in
some sort of loop and uses a C "continue" statement , but we can't know
whether that's always
the case?  Code that assumes that the outside looks a particular generally
violates modularity.
So another alternative would be to add more of the outside surrounding in
the function

It is hard to imagine that this function is called outside a loop that
> scans for directory records. So i thought it was necessary to talk
> about the loop controlling aspect.
>

It's okay describe how the code is typically (or always right now)  used,
but then give pseudo code inside that comment.


> Given the live rythm of this software, we need to place an unambiguous
> explanation for the programmers who look at this 10 years in the future.
> (Let's hope we are all still around then and can play Stadler and Waldorf.)
>
> So if my comment is not suitable for this, the replacement should still
> tell the reader that "true" means
>   "This is not a directory record. Re-loop, e.g. by continue, to get the
>    next real directory record or to see the end of the directory."
> and that "false" means
>   "Go on with what you want to do with this directory record."
>

This is better. So how about instead:

    Return true if this is not a directory record and  false if not.
?

That describes what it does. In the places that the function is called, it
is clear what happens actions are done when it is a directory record
or not, and that need not be spelled out.


>
> Minor concern:
>
> > - Alleged directory record spans over block limit.
> > + Directory record spans over block limit.
>
> It is not a directory record. It's just a few bytes which the code
> inspects whether they could be a directory record.
> If its potential length indicating byte says that the potential record
> spans over the block limit, then it is not such a record, but rather
> some rogue bytes which all should be 0. Thus "alleged record".
>

Ok. So now I understand what "alleged" means. I'd like something
unambiguous. So how about:


  Bytes read span over directory record block limit.

The other thing in the back of my mind was well, instead maybe the code
should not be so "eager" as you put it
so we need this jiggery. That might be a better solution.



> > I still don't understand what "below does not exactly round up." means.
>
> If the deviation of the necessary action from the arithmetical operation
> of rounding is not a stumble stone for the reader of the code, then we
> should simply trash that comment.
> I thought it was necessary because i spent a few minutes pondering
> over that special case.
> But if it causes unnecessary pondering by the reader, then it is no good.
>

I would like an accurate description of that assignment statement because it
is a bit weird. I've asked several times in what since is this "rounded"?


>
>
> Have a nice day :)
>
> Thomas
>
>
>