kicad-developers team mailing list archive

[Mark Roszko <mark.roszko@xxxxxxxxx>]

>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 <stambaughw@xxxxxxxxx> 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 <stambaughw@xxxxxxxxx> 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 <stambaughw@xxxxxxxxx> 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, 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 <stambaughw@xxxxxxxxx> 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@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
>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>>>
>>>
>>
>>
>>
>



-- 
Mark