Re: [Kicad-developers] [PATCH] add missing parentheses in page layout templates

[Wayne Stambaugh]

Patch pushed to master.  Thank you for your contribution to KiCad.

Cheers,

Wayne

On 9/8/2016 2:56 PM, Werner Almesberger wrote:
> The default and logo page layout templates are missing some opening
> parentheses. Eeschema's parser accepts them anyway, but it tripped
> my s-expr parser.
> 
> The gost templates and the built-in default in
> common/page_layout/page_layout_default_description.cpp
> are both correct.
> 
> - Werner
> 
> 
> 
> ___
> 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

Re: [Kicad-developers] [PATCH] add missing parentheses in page layout templates

[Wayne Stambaugh]

On 9/9/2016 4:49 PM, Mark Roszko wrote:
>> I'm not opposed to quoting all strings.  However the parser still needs
> to differentiate a string versus a keyword such as bold or italic used
> in the font definition.  Those should not be quoted.  I'm guessing
> you've covered that.
> 
> 
> Yep, it still has a concept of "keywords" (symbols in sexpr). It's
> just I made exceptions in the parser where it can be either due to the
> old formatter.
> 
> I have a more updated parser/formatter and more
> parsing (pcb too) in another tree/commit I have to rebase.

In the short term, I would rather leave the current parsers as is until
I'm more comfortable with your sexpr parser.  I could use your parser to
write the new schematic and symbol library file formats and then once
the dust has settled and it proves to be stable we can go back and
revisit the other parsers.

> 
> On Fri, Sep 9, 2016 at 4:45 PM, Wayne Stambaugh  wrote:
>> On 9/9/2016 3:24 PM, Mark Roszko wrote:
 I'm assuming this doesn't change the fp-lib-table file formatting.
>>>
>>> ~Mostly~ doesn't.
>>>
>>> One major difference is I absolutely quote all strings. Right now
>>> kicad's output formatter does not quote string types if there is only
>>> a single word. This violated the sanctity of what was a Symbol vs
>>> String in real S-expressions. So in that fp-lib-table implementation I
>>> linked, I added a workaround to GetSymbol or GetString for some atoms
>>> because of the old format being read in.
>>
>> I'm not opposed to quoting all strings.  However the parser still needs
>> to differentiate a string versus a keyword such as bold or italic used
>> in the font definition.  Those should not be quoted.  I'm guessing
>> you've covered that.
>>
>>>
>>>
 Why not make the PARSER object just work directly on an istream object
>>> or some abstraction that wraps input stream objects (not always
>>> necessarily based on std::istream)?  It really doesn't matter where the
>>> input stream comes from.  This just makes the parser more flexible.  You
>>> are only supporting string and file input streams at the moment.  There
>>> are a few wxInputStream objects
>>>
>>>
>>> No reason it doesn't, mainly because I avoid stream reading. If it
>>> accepted std::istream it would read it all into a string buffer before
>>> creating a complete in memory tree structure anyway.
>>> Nothing wrong with wxInputStream either, I was writing it to
>>> test/benchmark outside of KiCad first and so avoided wxWidgets
>>> dependencies (pretty hard to use wxWidgets in MSVC ;))
>>
>> This is something that I would like it place before I used it for
>> something critical such as the new schematic file format.  I like the
>> idea of using any stream input for parsing not just a select few.
>>
>>>
>>>
 There needs to be a SEXPR_TYPE_BOOL for boolean types.  AFIAR, the
>>> current parser supports yes, no, 0, 1, true, and false as boolean types.
>>>  I'm not sure there are any other types.
>>>
>>> Yea what I have was a playtest. It's easily implemented.
>>>
>>>
 Why are you not throwing an exception when there is a invalid key when
>>> parsing the fp-lib-table file?  Would there be any reason to accept an
>>> invalid file?
>>>
>>> Exception handling was still up in the air because its so the existing
>>> parser was tied to IO_ERROR and richio which was too tangled up to
>>> just use.
>>>
>>>
 Have you done any speed comparisons against the current parser design?
>>> I could live with a slight increase in parsing time although I would
>>> rather not.
>>>
>>> I did, but lost those results. I do believe when I benched pcb parsing
>>> vs my parser it was similar enough. But I'm not going to make any
>>> definite claims right now without the numbers at hand.
>>
>> I would like to confirm this before we head down this path.  It would
>> also be useful to test some different input stream objects to get an
>> idea of any performance issues before using it for any critical code.
>>
>>>
>>> On Fri, Sep 9, 2016 at 1:56 PM, Wayne Stambaugh  
>>> wrote:
 Looks interesting.  Coding policy issues aside, I have a few questions
 and comments:

 I'm assuming this doesn't change the fp-lib-table file formatting.

 Why not make the PARSER object just work directly on an istream object
 or some abstraction that wraps input stream objects (not always
 necessarily based on std::istream)?  It really doesn't matter where the
 input stream comes from.  This just makes the parser more flexible.  You
 are only supporting string and file input streams at the moment.  There
 are a few wxInputStream objects
 (http://docs.wxwidgets.org/3.0/classwx_input_stream.html) that could be
 useful.  I don't want to limit the input stream objects to just the few
 that the standard c++ library provides and I would rather not write our
 own if we don't have to.

 There needs to be a SEXPR_TYPE_BOOL for 

Re: [Kicad-developers] [PATCH] add missing parentheses in page layout templates

[Mark Roszko]

>I'm not opposed to quoting all strings.  However the parser still needs
to differentiate a string versus a keyword such as bold or italic used
in the font definition.  Those should not be quoted.  I'm guessing
you've covered that.


Yep, it still has a concept of "keywords" (symbols in sexpr). It's
just I made exceptions in the parser where it can be either due to the
old formatter.

I have a more updated parser/formatter and more
parsing (pcb too) in another tree/commit I have to rebase.

On Fri, Sep 9, 2016 at 4:45 PM, Wayne Stambaugh  wrote:
> On 9/9/2016 3:24 PM, Mark Roszko wrote:
>>> I'm assuming this doesn't change the fp-lib-table file formatting.
>>
>> ~Mostly~ doesn't.
>>
>> One major difference is I absolutely quote all strings. Right now
>> kicad's output formatter does not quote string types if there is only
>> a single word. This violated the sanctity of what was a Symbol vs
>> String in real S-expressions. So in that fp-lib-table implementation I
>> linked, I added a workaround to GetSymbol or GetString for some atoms
>> because of the old format being read in.
>
> I'm not opposed to quoting all strings.  However the parser still needs
> to differentiate a string versus a keyword such as bold or italic used
> in the font definition.  Those should not be quoted.  I'm guessing
> you've covered that.
>
>>
>>
>>> Why not make the PARSER object just work directly on an istream object
>> or some abstraction that wraps input stream objects (not always
>> necessarily based on std::istream)?  It really doesn't matter where the
>> input stream comes from.  This just makes the parser more flexible.  You
>> are only supporting string and file input streams at the moment.  There
>> are a few wxInputStream objects
>>
>>
>> No reason it doesn't, mainly because I avoid stream reading. If it
>> accepted std::istream it would read it all into a string buffer before
>> creating a complete in memory tree structure anyway.
>> Nothing wrong with wxInputStream either, I was writing it to
>> test/benchmark outside of KiCad first and so avoided wxWidgets
>> dependencies (pretty hard to use wxWidgets in MSVC ;))
>
> This is something that I would like it place before I used it for
> something critical such as the new schematic file format.  I like the
> idea of using any stream input for parsing not just a select few.
>
>>
>>
>>> There needs to be a SEXPR_TYPE_BOOL for boolean types.  AFIAR, the
>> current parser supports yes, no, 0, 1, true, and false as boolean types.
>>  I'm not sure there are any other types.
>>
>> Yea what I have was a playtest. It's easily implemented.
>>
>>
>>> Why are you not throwing an exception when there is a invalid key when
>> parsing the fp-lib-table file?  Would there be any reason to accept an
>> invalid file?
>>
>> Exception handling was still up in the air because its so the existing
>> parser was tied to IO_ERROR and richio which was too tangled up to
>> just use.
>>
>>
>>> Have you done any speed comparisons against the current parser design?
>> I could live with a slight increase in parsing time although I would
>> rather not.
>>
>> I did, but lost those results. I do believe when I benched pcb parsing
>> vs my parser it was similar enough. But I'm not going to make any
>> definite claims right now without the numbers at hand.
>
> I would like to confirm this before we head down this path.  It would
> also be useful to test some different input stream objects to get an
> idea of any performance issues before using it for any critical code.
>
>>
>> On Fri, Sep 9, 2016 at 1:56 PM, Wayne Stambaugh  wrote:
>>> Looks interesting.  Coding policy issues aside, I have a few questions
>>> and comments:
>>>
>>> I'm assuming this doesn't change the fp-lib-table file formatting.
>>>
>>> Why not make the PARSER object just work directly on an istream object
>>> or some abstraction that wraps input stream objects (not always
>>> necessarily based on std::istream)?  It really doesn't matter where the
>>> input stream comes from.  This just makes the parser more flexible.  You
>>> are only supporting string and file input streams at the moment.  There
>>> are a few wxInputStream objects
>>> (http://docs.wxwidgets.org/3.0/classwx_input_stream.html) that could be
>>> useful.  I don't want to limit the input stream objects to just the few
>>> that the standard c++ library provides and I would rather not write our
>>> own if we don't have to.
>>>
>>> There needs to be a SEXPR_TYPE_BOOL for boolean types.  AFIAR, the
>>> current parser supports yes, no, 0, 1, true, and false as boolean types.
>>>  I'm not sure there are any other types.
>>>
>>> Why are you not throwing an exception when there is a invalid key when
>>> parsing the fp-lib-table file?  Would there be any reason to accept an
>>> invalid file?
>>>
>>> Have you done any speed comparisons against the current parser design?
>>> I could live with a slight increase in parsing time although I would

Re: [Kicad-developers] [PATCH] add missing parentheses in page layout templates

[Wayne Stambaugh]

On 9/9/2016 3:24 PM, Mark Roszko wrote:
>> I'm assuming this doesn't change the fp-lib-table file formatting.
> 
> ~Mostly~ doesn't.
> 
> One major difference is I absolutely quote all strings. Right now
> kicad's output formatter does not quote string types if there is only
> a single word. This violated the sanctity of what was a Symbol vs
> String in real S-expressions. So in that fp-lib-table implementation I
> linked, I added a workaround to GetSymbol or GetString for some atoms
> because of the old format being read in.

I'm not opposed to quoting all strings.  However the parser still needs
to differentiate a string versus a keyword such as bold or italic used
in the font definition.  Those should not be quoted.  I'm guessing
you've covered that.

> 
> 
>> Why not make the PARSER object just work directly on an istream object
> or some abstraction that wraps input stream objects (not always
> necessarily based on std::istream)?  It really doesn't matter where the
> input stream comes from.  This just makes the parser more flexible.  You
> are only supporting string and file input streams at the moment.  There
> are a few wxInputStream objects
> 
> 
> No reason it doesn't, mainly because I avoid stream reading. If it
> accepted std::istream it would read it all into a string buffer before
> creating a complete in memory tree structure anyway.
> Nothing wrong with wxInputStream either, I was writing it to
> test/benchmark outside of KiCad first and so avoided wxWidgets
> dependencies (pretty hard to use wxWidgets in MSVC ;))

This is something that I would like it place before I used it for
something critical such as the new schematic file format.  I like the
idea of using any stream input for parsing not just a select few.

> 
> 
>> There needs to be a SEXPR_TYPE_BOOL for boolean types.  AFIAR, the
> current parser supports yes, no, 0, 1, true, and false as boolean types.
>  I'm not sure there are any other types.
> 
> Yea what I have was a playtest. It's easily implemented.
> 
> 
>> Why are you not throwing an exception when there is a invalid key when
> parsing the fp-lib-table file?  Would there be any reason to accept an
> invalid file?
> 
> Exception handling was still up in the air because its so the existing
> parser was tied to IO_ERROR and richio which was too tangled up to
> just use.
> 
> 
>> Have you done any speed comparisons against the current parser design?
> I could live with a slight increase in parsing time although I would
> rather not.
> 
> I did, but lost those results. I do believe when I benched pcb parsing
> vs my parser it was similar enough. But I'm not going to make any
> definite claims right now without the numbers at hand.

I would like to confirm this before we head down this path.  It would
also be useful to test some different input stream objects to get an
idea of any performance issues before using it for any critical code.

> 
> On Fri, Sep 9, 2016 at 1:56 PM, Wayne Stambaugh  wrote:
>> Looks interesting.  Coding policy issues aside, I have a few questions
>> and comments:
>>
>> I'm assuming this doesn't change the fp-lib-table file formatting.
>>
>> Why not make the PARSER object just work directly on an istream object
>> or some abstraction that wraps input stream objects (not always
>> necessarily based on std::istream)?  It really doesn't matter where the
>> input stream comes from.  This just makes the parser more flexible.  You
>> are only supporting string and file input streams at the moment.  There
>> are a few wxInputStream objects
>> (http://docs.wxwidgets.org/3.0/classwx_input_stream.html) that could be
>> useful.  I don't want to limit the input stream objects to just the few
>> that the standard c++ library provides and I would rather not write our
>> own if we don't have to.
>>
>> There needs to be a SEXPR_TYPE_BOOL for boolean types.  AFIAR, the
>> current parser supports yes, no, 0, 1, true, and false as boolean types.
>>  I'm not sure there are any other types.
>>
>> Why are you not throwing an exception when there is a invalid key when
>> parsing the fp-lib-table file?  Would there be any reason to accept an
>> invalid file?
>>
>> Have you done any speed comparisons against the current parser design?
>> I could live with a slight increase in parsing time although I would
>> rather not.
>>
>> On 9/9/2016 11:21 AM, Mark Roszko wrote:
>>> If you want a peek at something I've played with:
>>>
>>> https://github.com/marekr/kicad-sexpr/commit/3bf73b984eaeef3d8aede351592a2cc07677cb10
>>>
>>>
>>> I avoid boost like the plague ^^
>>>
>>> On Fri, Sep 9, 2016 at 10:55 AM, Wayne Stambaugh  
>>> wrote:
 I understand completely.  I've written enough of our sexpr code to know
 how it works and where it's strengths and weaknesses lie.  I just want
 to make sure broken file formats are flagged as such by any parsers that
 we write.  If our output formatters are creating these files, that is an
 all 

Re: [Kicad-developers] [PATCH] add missing parentheses in page layout templates

[Mark Roszko]

>I'm assuming this doesn't change the fp-lib-table file formatting.

~Mostly~ doesn't.

One major difference is I absolutely quote all strings. Right now
kicad's output formatter does not quote string types if there is only
a single word. This violated the sanctity of what was a Symbol vs
String in real S-expressions. So in that fp-lib-table implementation I
linked, I added a workaround to GetSymbol or GetString for some atoms
because of the old format being read in.


>Why not make the PARSER object just work directly on an istream object
or some abstraction that wraps input stream objects (not always
necessarily based on std::istream)?  It really doesn't matter where the
input stream comes from.  This just makes the parser more flexible.  You
are only supporting string and file input streams at the moment.  There
are a few wxInputStream objects


No reason it doesn't, mainly because I avoid stream reading. If it
accepted std::istream it would read it all into a string buffer before
creating a complete in memory tree structure anyway.
Nothing wrong with wxInputStream either, I was writing it to
test/benchmark outside of KiCad first and so avoided wxWidgets
dependencies (pretty hard to use wxWidgets in MSVC ;))


>There needs to be a SEXPR_TYPE_BOOL for boolean types.  AFIAR, the
current parser supports yes, no, 0, 1, true, and false as boolean types.
 I'm not sure there are any other types.

Yea what I have was a playtest. It's easily implemented.


>Why are you not throwing an exception when there is a invalid key when
parsing the fp-lib-table file?  Would there be any reason to accept an
invalid file?

Exception handling was still up in the air because its so the existing
parser was tied to IO_ERROR and richio which was too tangled up to
just use.


>Have you done any speed comparisons against the current parser design?
I could live with a slight increase in parsing time although I would
rather not.

I did, but lost those results. I do believe when I benched pcb parsing
vs my parser it was similar enough. But I'm not going to make any
definite claims right now without the numbers at hand.

On Fri, Sep 9, 2016 at 1:56 PM, Wayne Stambaugh  wrote:
> Looks interesting.  Coding policy issues aside, I have a few questions
> and comments:
>
> I'm assuming this doesn't change the fp-lib-table file formatting.
>
> Why not make the PARSER object just work directly on an istream object
> or some abstraction that wraps input stream objects (not always
> necessarily based on std::istream)?  It really doesn't matter where the
> input stream comes from.  This just makes the parser more flexible.  You
> are only supporting string and file input streams at the moment.  There
> are a few wxInputStream objects
> (http://docs.wxwidgets.org/3.0/classwx_input_stream.html) that could be
> useful.  I don't want to limit the input stream objects to just the few
> that the standard c++ library provides and I would rather not write our
> own if we don't have to.
>
> There needs to be a SEXPR_TYPE_BOOL for boolean types.  AFIAR, the
> current parser supports yes, no, 0, 1, true, and false as boolean types.
>  I'm not sure there are any other types.
>
> Why are you not throwing an exception when there is a invalid key when
> parsing the fp-lib-table file?  Would there be any reason to accept an
> invalid file?
>
> Have you done any speed comparisons against the current parser design?
> I could live with a slight increase in parsing time although I would
> rather not.
>
> On 9/9/2016 11:21 AM, Mark Roszko wrote:
>> If you want a peek at something I've played with:
>>
>> https://github.com/marekr/kicad-sexpr/commit/3bf73b984eaeef3d8aede351592a2cc07677cb10
>>
>>
>> I avoid boost like the plague ^^
>>
>> On Fri, Sep 9, 2016 at 10:55 AM, Wayne Stambaugh  
>> wrote:
>>> I understand completely.  I've written enough of our sexpr code to know
>>> how it works and where it's strengths and weaknesses lie.  I just want
>>> to make sure broken file formats are flagged as such by any parsers that
>>> we write.  If our output formatters are creating these files, that is an
>>> all together different issue that needs to be addressed.
>>>
>>> At some point, if I ever get the time, I would like to review this.  It
>>> might be useful use a property tree like parser/formatter such as how
>>> xml is handled or some other similar construct rather than the way we
>>> are doing it now.  I took a look at the boost::property_tree class it
>>> looks like a possible candidate.  I was hoping to do something like this
>>> for the new schematic file format.  It's primarily a issue of finding
>>> the free time to learn something new at the moment.
>>>
>>> On 09/09/2016 10:38 AM, Mark Roszko wrote:
 the existing parser is "by the seat of our pants" style parsing
 where there is no centralized parsing logic and instead
 manual definition of expected tokens for every single parsable atom
 and well, 

Re: [Kicad-developers] [PATCH] add missing parentheses in page layout templates

[Mark Roszko]

the existing parser is "by the seat of our pants" style parsing
where there is no centralized parsing logic and instead
manual definition of expected tokens for every single parsable atom
and well, this has led to places where expectRightParenthesis() isn't
called and the like. But that won't break anything because it
continues walking down the token list after the case for an atom is
executed.

Meh, I probably suck at explaining ^

Looks like I forgot to submit a patch for these files as I submitted
one for another template file before that I think JP commited. Because
my strict SEXPR parser barked :3

On Fri, Sep 9, 2016 at 10:19 AM, Wayne Stambaugh  wrote:
> Hmm.  I took another look at this patch and I'm concerned as to why the
> page layout template parser did not choke on this.  That should be fixed
> as well.
>
> On 9/8/2016 2:56 PM, Werner Almesberger wrote:
>> The default and logo page layout templates are missing some opening
>> parentheses. Eeschema's parser accepts them anyway, but it tripped
>> my s-expr parser.
>>
>> The gost templates and the built-in default in
>> common/page_layout/page_layout_default_description.cpp
>> are both correct.
>>
>> - Werner
>>
>>
>>
>> ___
>> 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



-- 
Mark

___
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

Re: [Kicad-developers] [PATCH] add missing parentheses in page layout templates

[Wayne Stambaugh]

Hmm.  I took another look at this patch and I'm concerned as to why the
page layout template parser did not choke on this.  That should be fixed
as well.

On 9/8/2016 2:56 PM, Werner Almesberger wrote:
> The default and logo page layout templates are missing some opening
> parentheses. Eeschema's parser accepts them anyway, but it tripped
> my s-expr parser.
> 
> The gost templates and the built-in default in
> common/page_layout/page_layout_default_description.cpp
> are both correct.
> 
> - Werner
> 
> 
> 
> ___
> 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

Re: [Kicad-developers] [PATCH] add missing parentheses in page layout templates

[Clemens Koller]

On 2016-09-08 20:56, Werner Almesberger wrote:
> The default and logo page layout templates are missing some opening
> parentheses. Eeschema's parser accepts them anyway, but it tripped
> my s-expr parser.

Is accepting a wrong syntax without a warning not a bug worth fixing?

Regards,

Clemens


___
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