kicad-developers team mailing list archive

[Tiger12506 <tiger12506@xxxxxxxxx>]

My opinion doesn't matter, but how about this for a comment...

// ReadName has a side-effect of incrementing the parser, so it should 
indeed be called twice.

I certainly agree with you that people shouldn't change something they 
don't understand, but I also don't think it's unreasonable for someone 
to think that a function named ReadName might be a harmless read-only 
function.

Or if that comment doesn't accurately describe what's happening, provide 
an example of what's getting parsed by that line in a comment.

Just a thought.


On 4/13/2017 6:06 PM, Cirilo Bernardo wrote:
> On Thu, Apr 13, 2017 at 1:34 PM, Clemens Koller <cko@xxxxxxxxx> wrote:
> > Hi!
> >
> > These lines scream for some comments in the source...
> > I wouldn't get it, too.
> >
> > Regards,
> >
> > Clemens
> What sort of comment: "this is really supposed to have two sequential
> calls to the same function, so don't change it"?  For me that makes no
> sense. If anyone is going to play with parsers they should be familiar
> with the standard that is being implemented; making changes without
> understanding the specification (and without understanding what the
> parser is doing) cannot possibly be a good thing. Programmers should
> also check that they are fixing a demonstrable problem and if they don't
> understand the code then it should be left alone. At any rate there is a
> comment only a few lines back about how the unimplemented features
> PROTO and EXTERNPROTO are to be treated and from this the fact
> that PROTO and EXTERNPROTO code are different and in two distinct
> blocks should suggest that they really should not be the same code.
> In the past when other developers have seen code that they don't
> immediately understand but which looks a little strange to them, they
> at least ask for comments rather than making blind changes. If people
> don't read an obvious comment block a few lines up I don't see why they
> would read comments immediately surrounding code either. How much
> hand-holding are we expected to do?
>
> - Cirilo
>
>
> > On 2017-04-13 14:03, Wayne Stambaugh wrote:
> > > Cirilo,
> > >
> > > Thanks for the info.  The second call to ReadName() does look a bit odd
> > > so I can understand Konrad's confusion.
> > >
> > > Cheers,
> > >
> > > Wayne
> > >
> > > On 4/12/2017 6:12 PM, Cirilo Bernardo wrote:
> > > > Do not accept this patch, it will break the parser. The statement
> > > > which was removed is not redundant.
> > > >
> > > > - Cirilo
> > > >
> > > > On Wed, Apr 12, 2017 at 8:01 PM, Konrad Beckmann
> > > > <konrad.beckmann@xxxxxxxxx> wrote:
> > > > > ---
> > > > >   plugins/3d/vrml/v2/vrml2_base.cpp | 2 +-
> > > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > >
> > > > > _______________________________________________
> > > > > Mailing list: https://launchpad.net/~kicad-developers
> > > > > Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> > > > > Unsubscribe : https://launchpad.net/~kicad-developers
> > > > > More help   : https://help.launchpad.net/ListHelp
> > > > _______________________________________________
> > > > Mailing list: https://launchpad.net/~kicad-developers
> > > > Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> > > > Unsubscribe : https://launchpad.net/~kicad-developers
> > > > More help   : https://help.launchpad.net/ListHelp
> > > _______________________________________________
> > > Mailing list: https://launchpad.net/~kicad-developers
> > > Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> > > Unsubscribe : https://launchpad.net/~kicad-developers
> > > More help   : https://help.launchpad.net/ListHelp
> > _______________________________________________
> > Mailing list: https://launchpad.net/~kicad-developers
> > Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> > Unsubscribe : https://launchpad.net/~kicad-developers
> > More help   : https://help.launchpad.net/ListHelp
> _______________________________________________
> Mailing list: https://launchpad.net/~kicad-developers
> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp