>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 <[email protected]> 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 <[email protected]> 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 <[email protected]> >>>> 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 <[email protected]> >>>>>> 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 : [email protected] >>>>>>>> Unsubscribe : https://launchpad.net/~kicad-developers >>>>>>>> More help : https://help.launchpad.net/ListHelp >>>>>>>> >>>>>>> >>>>>>> >>>>>>> _______________________________________________ >>>>>>> Mailing list: https://launchpad.net/~kicad-developers >>>>>>> Post to : [email protected] >>>>>>> Unsubscribe : https://launchpad.net/~kicad-developers >>>>>>> More help : https://help.launchpad.net/ListHelp >>>>>> >>>>>> >>>>>> >>>> >>>> >>>> >>> >> >> >> > -- Mark _______________________________________________ Mailing list: https://launchpad.net/~kicad-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
