On 4/13/2017 9:58 PM, Cirilo Bernardo wrote: > On Thu, Apr 13, 2017 at 11:55 PM, Tiger12506 <tiger12...@gmail.com> wrote: >> 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. >> >> > > I'll think about it; maybe I can put the expected pattern into a comment so > it's more obvious why the function is called twice. > > - Cirilo
Cirilo, I wouldn't go overboard. A simple comment stating the double call is correct and removing it will break the parser. Honestly, I'm OK if you don't add the comment. If I would have seen this, I would have asked before making any changes. I don't expect you to do any serious hand holding. Thanks, Wayne > >> >> On 4/13/2017 6:06 PM, Cirilo Bernardo wrote: >>> >>> On Thu, Apr 13, 2017 at 1:34 PM, Clemens Koller <c...@embeon.de> 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.beckm...@gmail.com> 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@lists.launchpad.net >>>>>>> Unsubscribe : https://launchpad.net/~kicad-developers >>>>>>> More help : https://help.launchpad.net/ListHelp >>>>>>> >>>>>> _______________________________________________ >>>>>> Mailing list: https://launchpad.net/~kicad-developers >>>>>> Post to : kicad-developers@lists.launchpad.net >>>>>> Unsubscribe : https://launchpad.net/~kicad-developers >>>>>> More help : https://help.launchpad.net/ListHelp >>>>>> >>>>> _______________________________________________ >>>>> Mailing list: https://launchpad.net/~kicad-developers >>>>> Post to : kicad-developers@lists.launchpad.net >>>>> Unsubscribe : https://launchpad.net/~kicad-developers >>>>> More help : https://help.launchpad.net/ListHelp >>>>> >>>> _______________________________________________ >>>> Mailing list: https://launchpad.net/~kicad-developers >>>> Post to : kicad-developers@lists.launchpad.net >>>> Unsubscribe : https://launchpad.net/~kicad-developers >>>> More help : https://help.launchpad.net/ListHelp >>> >>> _______________________________________________ >>> Mailing list: https://launchpad.net/~kicad-developers >>> Post to : kicad-developers@lists.launchpad.net >>> Unsubscribe : https://launchpad.net/~kicad-developers >>> More help : https://help.launchpad.net/ListHelp >> >> >> >> _______________________________________________ >> Mailing list: https://launchpad.net/~kicad-developers >> Post to : kicad-developers@lists.launchpad.net >> Unsubscribe : https://launchpad.net/~kicad-developers >> More help : https://help.launchpad.net/ListHelp > > _______________________________________________ > Mailing list: https://launchpad.net/~kicad-developers > Post to : kicad-developers@lists.launchpad.net > Unsubscribe : https://launchpad.net/~kicad-developers > More help : https://help.launchpad.net/ListHelp > _______________________________________________ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
