Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: x/tools/cmd/goyacc: reduce dependence on globals #44774

Open
SteelPhase opened this issue Mar 3, 2021 · 7 comments · May be fixed by golang/tools#282
Open

proposal: x/tools/cmd/goyacc: reduce dependence on globals #44774

SteelPhase opened this issue Mar 3, 2021 · 7 comments · May be fixed by golang/tools#282
Labels
Projects
Milestone

Comments

@SteelPhase
Copy link

@SteelPhase SteelPhase commented Mar 3, 2021

background

Code generated by goyacc has a dependence on package globals that it doesn't need to have.

yyDebug, and yyErrorVerbose do not specifically need to be globals, and can be variables on the parser instead.

I did not find documentation for bison/yacc that states YYDEBUG or a YYERRORVERBOSE need to be made available inside grammar actions.

We can follow java here and expose them via the Parser which is already technically available to grammar actions but not documented.

description

I'm proposing the following

  • Move yyDebug, and yyErrorVerbose into yyParserImpl
  • For compatibility, modify yyNewParser to grab the value of the package globals, and set them internally when called
    • something like `return &$$ParserImpl{ debug: $$Debug, errorVerbose: $$ErrorVerbose}
  • Create a new yyNewParser### function that exposed debug, and errorVerbose as inputs
  • Create a new yyParse### function that exposed debug, and errorVerbose as inputs
  • Modify the yyParser interface to expose, and allow modification of, the internal state of debug, and errorVerbose
  • Modify yyParserImpl to match the updated yyParser interface

drawbacks

There is potential that if someone had already written a customer parser to match yyParser their parser would no longer match the interface

Any grammar actions that depend on yyDebug or yyErrorVerbose would break. A key point here is this behavior doesn't seem to be well defined
anyways, and I'm not sure if the intention was to make them available in grammar actions.

@gopherbot gopherbot added this to the Proposal milestone Mar 3, 2021
@SteelPhase SteelPhase linked a pull request that will close this issue Mar 3, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 3, 2021

Change https://golang.org/cl/298389 mentions this issue: goyacc: reduce dependence on globals

Loading

@rogpeppe
Copy link
Contributor

@rogpeppe rogpeppe commented Mar 3, 2021

I'm not entirely certain that this is worth doing. I know it's not totally ideal that these settings are global, but I'd be interested to hear about an actual scenario where the settings need to vary dynamically for a given grammar.

Specifically:

  • yyDebug seems like something that you'll only want to enable when debugging the grammar during development.
  • yyErrorVerbose seems like something that would be reasonable to make a one-time decision for, rather than dynamically changing it.

Loading

@SteelPhase
Copy link
Author

@SteelPhase SteelPhase commented Mar 3, 2021

In my case I have other structs that rely on the generated parser at runtime. I've exposed the ability to change these settings at that level, but because the values are global, I can't guarantee that 1 instance wouldn't clobber another.

Loading

@rogpeppe
Copy link
Contributor

@rogpeppe rogpeppe commented Mar 4, 2021

I've exposed the ability to change these settings at that level

Perhaps you should not have done that? ISTM that it might be better to choose those values statically when generating the code, or to expose them as globals.

I'm still interested to hear a concrete use case for why these settings would really benefit from being dynamically settable - i.e. why would the clients of your library want to do this?

Loading

@SteelPhase
Copy link
Author

@SteelPhase SteelPhase commented Mar 4, 2021

I had two requirements for this. I have a path that is meant to be as performant as possible, and a path for gathering simulation, and validation information. I need these settings in both of these states for either path. I'm pretty sure there is no work around. I've gone ahead and pulled a copy of goyacc, into my project, and made the changes I needed to, for this to work.

If no one else sees getting rid of these globals as a benefit it doesn't need to be pulled into golang/tools.

Loading

@rogpeppe
Copy link
Contributor

@rogpeppe rogpeppe commented Mar 4, 2021

What I don't understand is why you might need both of those paths to be enabled concurrently.

Loading

@SteelPhase
Copy link
Author

@SteelPhase SteelPhase commented Mar 4, 2021

I really don't want to get into the weeks of the project, but in this scenario, these two paths can be used by the same project. The current cause of this strive was caused by an api that offers endpoints that each need one or the other. One for performance reason, and the other for diagnostics.

Loading

@ianlancetaylor ianlancetaylor changed the title proposal: tools/cmd/goyacc: reduce dependence on globals proposal: x/tools/cmd/goyacc: reduce dependence on globals Mar 4, 2021
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Mar 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Proposals
Incoming
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants