Re: [PATCH 0/4] RFC: add stats about parsing

[Adrian Vogelsgesang]

Hi Akim,

>> For my projects, I don’t care which exact option is pursued since
>> I never use `yydebug` in commited code. I only add debug
>> code temporarily, fix the bug, and then remove the debug code
>> again before commiting anything to master.  
> Ok.  Which means you commit the generated files?

No, I commit the bison grammar and generate the file as part of the CMake
build steps.
I only never commit a grammar which contains debug code, i.e.m I never commit
a grammar which depends on `yydebug` or any other debugging functionality.
If I use `yydebug` to investigate a defect, I remove that debug code again 
before
commiting the fixed grammar.

>> One question regarding variant 3a:
>> What would be the outcome of the combination
>>> %define parse.trace false
>>>> %define parse.stats true 
> I don't like 3a, I prefer 3b :)  But in the case you quote,
> I would probably make it an error: you cannot set parse.stats
> if parse.trace is not enabled.
>
> But then again, I mentioned 3a only to explain the commit.
> I should have emphasized that it's a bad solution: enhancing
> parse.trace is better imho.

If this combination would be an error, I also prefer 3b over 3a.
Instead of checking for a contradiction, let's build the interface in such a
way that it's syntactically impossible to get into that error state.

My personal ranking would be 2a (best), 3b, 2b (worst),
I like 2b the least since it would break compatibility by outputting different 
traces.
I prefer 2a over 3b since 2a provides more control from within the program: I 
can 
switch between traditional traces and stats.

Cheers,
Adrian


On 03/07/2019, 22:45, "Akim Demaille" <address@hidden> wrote:

    Hi Adrian,
    
    Thanks for the feedback.
    
    > Le 3 juil. 2019 à 14:08, Adrian Vogelsgesang <address@hidden> a écrit :
    > 
    > Hi Akim,
    > 
    > collecting statistics about a parse sounds like a great idea to me
    > and I am looking forward to it.
    > Thanks for the time and effort you spent on this! :)
    
    Well, on this particular feature, I was driven by own needs :)
    But thank you.
    
    
    > For my projects, I don’t care which exact option is pursued since
    > I never use `yydebug` in commited code. I only add debug
    > code temporarily, fix the bug, and then remove the debug code
    > again before commiting anything to master.
    
    Ok.  Which means you commit the generated files?
    
    > I don’t think anyone should have a dependency in production
    > code on `yydebug` behavior.
    
    Well, Bison, when installed, does keep the parser traces on
    (and you can trigger them with --trace=parse).
    
    But you are right: this is debatable.  I should probably stop
    doing this in the generated tarball.  It's ok for the git repo,
    but maybe the generate tarballs should not have it enabled by
    default.
    
    > Hence, I guess breaking the
    > POSIX standard here would be fine.
    > Still, I share your concerns about breaking standards.
    > 
    > Hence, I would be in favour of variant 2 or 3.
    
    Thanks for the feedback!
    
    > One question regarding variant 3a:
    > What would be the outcome of the combination
    >> %define parse.trace false
    >> %define parse.stats true
    
    I don't like 3a, I prefer 3b :)  But in the case you quote,
    I would probably make it an error: you cannot set parse.stats
    if parse.trace is not enabled.
    
    But then again, I mentioned 3a only to explain the commit.
    I should have emphasized that it's a bad solution: enhancing
    parse.trace is better imho.