kicad-developers team mailing list archive

[Wayne Stambaugh <stambaughw@xxxxxxxxx>]

On 4/13/2017 9:58 PM, Cirilo Bernardo wrote:
> On Thu, Apr 13, 2017 at 11:55 PM, Tiger12506 <tiger12506@xxxxxxxxx> 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 <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
>>
>>
>>
>> _______________________________________________
>> 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
>