Re: Enhancement request: enabling Variant in C parsers

[Akim Demaille]

Hi Victor,

Thanks for your answer.  Please, keep all this public, keep the CC.

I don’t have time right time to answer in details, I’ll do that
during the weekend.  I also want to wrap 3.1 with what we have
now.

Thanks!

> Le 19 août 2018 à 22:11, Victor Khomenko <address@hidden> a écrit :
> 
> Hi Akim,
> 
>>> I did look at it - this example scared me off c++ parsers :-(
>> 
>> Really???  Then I failed.  Can you be more specific?
> 
> Ok, I'll try to make some suggestions for the manual and bison. Please note 
> that much of what I say might be due to lack of understanding - I'm just a 
> regular bison user, and don't understand the skeletons, internals, etc. The 
> context is that I have 8 bison C-style pure parsers in my program, most of 
> them are very simple, but a couple are fairly complicated, including a glr 
> one.
> 
> 1. The C++ section in the bison manual starts with the description of 
> generated files: position.hh, location.hh, stack.hh, file.hh and file.cc. 
> That's 5 generated files, so at this point many prospective users would just 
> run away screaming. I'd say the first three files are completely unnecessary 
> and can be dumped into file.hh (or even .cc) - note that the contents could 
> be in some bison::detail namespace, so that the types are exactly the same in 
> all parsers and the linker can merge the functions if they are declared as 
> inline but were not actually inlined. The work-around for multiple parsers 
> suggested in Sect. 10.1.3.3 (making one parser generate these includes in 
> master/ and the other parsers use them) is ugly and introduces unwanted 
> include and build dependencies (e.g. there could be two independent libraries 
> within a project, each using a parser; this technique would make them 
> dependent both for includes and the built order).
> 
> I'd say file.hh can also be suppressed in simple cases, e.g. if one replaces 
> the generated class by a function (see below) - it's then possible to declare 
> the parser function at the point of use.
> 
> 2. int yylex (semantic type* yylval, location type* yylloc, type1 arg1, ...) 
> [Method on parser]
> In C++ one would usually pass the parameters by references rather than 
> pointers - also to avoid handling null-pointers in yylex.
> 
> 4. calc++ is a rather complicated example, for warming up it would be better 
> to use something simple. Then calc++ would be good to illustrate various 
> intricacies like mutual #include dependencies and a flex scanner.
> 
> 3. the destructor of calcxx_driver should not be virtual; in fact, the 
> automatically generated destructor should be ok, so no need to declare it at 
> all. The constructor is not necessary either - since C++11 one can give the 
> default initial values to data members:
>    std::map<std::string, int> variables={ {"one", 1}, {"two", 2} };
>    trace_scanning=false;
>    trace_parsing=false;
> Maybe some other functions within the class, like error(), can be defined at 
> the point of declaration.
> 
> 4. int calcxx_driver::parse (const std::string& f);
> This is confusing, as the parser also has the function with the same name, 
> but no parameters. Maybe call it parse_file?
> 
> 5. scan_begin() and scan_end() could be removed to simplify the interface, 
> and inlined into parse(...).
> 
> 6. implementing calcxx_driver::scan_begin() and calcxx_driver::scan_end() in 
> the lexer file is really bad - I guess one can easily implement them in the 
> calcxx_driver.cpp file - this does not cause any problems with #includes.
> 
> 7. it would be better to use C++ streams rather than C-style fopen/fclose 
> (note that error() does use streams).
> 
>> Note that you don’t have anything to do to
>> reset the parser, all the problem is actually the scanner. 
> 
> That's good - I suggest to say this explicitly in the documentation.
> 
> 
>> The parser object holds no state, everything is local to yyparse.
> 
> That's a surprise to me! (and needs to be said in the manual) Well, this is 
> actually very nice, but also means that the parser class is used as a 
> namespace, so could/should be converted to one (or just dissolved, with its 
> content ejected into the surrounding yy namespace, thus shortening those long 
> multiply-qualified names), and parse() becomes just a function. The 
> user-declared parameters can then be passed to yyparse() directly, i.e. we 
> get something similar to the old style pure parsers. 
> 
> 
>> I’ll try to look at this section with a fresh eye for the following release, 
>> and fix
>> these issues.  Help will be deeply appreciated :)
> 
> I hope some of the above could help, please ignore the silly bits.
> 
> Cheers,
> Victor.
> 
> 
> 
> 
> 
> 
> 
> 
> 
>