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

Configure dart debug flags through environment declaration #3128

Merged
merged 1 commit into from
Oct 11, 2021
Merged

Configure dart debug flags through environment declaration #3128

merged 1 commit into from
Oct 11, 2021

Conversation

canastro
Copy link
Contributor

@canastro canastro commented Mar 17, 2021

Issue: #3127

Allow the user to configure which debug loggings he wants to see without having to run antlr locally.

@canastro
Copy link
Contributor Author

@lingyv-li please take a look if this is an ok change.

Copy link
Member

@lingyv-li lingyv-li left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes looks good to me, thanks for adding this.

@kaby76
Copy link
Contributor

kaby76 commented Mar 18, 2021

This would be nice feature to apply across all targets so as to be a consistent feature. I'm working with all targets. When I am trying to debug the runtime, I would like to be able to turn on debug first and get some quick feedback. Otherwise, I will have to build a version of the runtime. In C#, and likely Java, it's basically impossible to set debug dynamically because the value is static final const. The value is used at program load time to JIT compile out the debugging output code, and thus impossible to turn on from a switch to Parser. --Ken

@ericvergnaud
Copy link
Contributor

This would be nice feature to apply across all targets so as to be a consistent feature. I'm working with all targets. When I am trying to debug the runtime, I would like to be able to turn on debug first and get some quick feedback. Otherwise, I will have to build a version of the runtime. In C#, and likely Java, it's basically impossible to set debug dynamically because the value is static final const. The value is used at program load time to JIT compile out the debugging output code, and thus impossible to turn on from a switch to Parser. --Ken

Unfortunately it would impact performance. Using a static final lets the compiler drop dead code, which would not be possible if the flag was read from outside.

@kaby76
Copy link
Contributor

kaby76 commented Mar 19, 2021

Unfortunately it would impact performance. Using a static final lets the compiler drop dead code, which would not be possible if the flag was read from outside.

From what I've read of this in an Issue posted on the compiler for C#, the assembly initializers might be performed first, then jited for exactly this reason. I would point you to what Ayers at MS said, but i can't remember. It would have to be checked because the issue was open when I read it a month ago. But why would the Dart code have debug = true to begin with?

@lingyv-li
Copy link
Member

But why would the Dart code have debug = true to begin with?

That clearly is a mistake by me! Thanks for catching the bug too @canastro 😃

On the performance note for Dart, would it be better to also change the flags to const for parser?

@kaby76
Copy link
Contributor

kaby76 commented Mar 22, 2021

On the performance note for Dart, would it be better to also change the flags to const for parser?

I don't know what the Dart compiler generates. Does the debug code compile away with or without const? That would be the deciding guide I suppose. I would think it should just be like all the other targets. It's hard for me to context switch between 7 different runtimes now.

Copy link
Member

@lingyv-li lingyv-li left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was misled by the name fromEnvironment, it turns out to be not "from the environment variables" but "the environment declaration" provided when compiling (AOT) or running. And that's why it's returns a compile-time constant value.

With this I believe at least for Dart target it won't affect performance. Though I don't think other languages can have this without impacting performance. Good news is there is an explicit defaultValue so you don't need to remember how to set it if you forgot.

See: dart-lang/sdk#27585

doc/dart-target.md Outdated Show resolved Hide resolved
runtime/Dart/lib/src/atn/src/parser_atn_simulator.dart Outdated Show resolved Hide resolved
@canastro canastro requested a review from lingyv-li March 24, 2021 10:56
Copy link
Member

@lingyv-li lingyv-li left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@canastro canastro changed the title Configure dart debug flags through environment variables Configure dart debug flags through environment declaration Mar 30, 2021
@stefanschaller
Copy link

@ericvergnaud Same here. The PR looks great, @lingyv-li approved the PR, all steps from the pipeline are passing (Of course not the go-runtime).
We definitely need those features in the next version on pub.dev that should be published ASAP.
Would be so great, because I really need those features and fixes.

@canastro
Copy link
Contributor Author

@ericvergnaud @lingyv-li just rebased and made this PR green and good to go

@lingyv-li
Copy link
Member

@parrt can you please check and merge this? Thanks.

doc/dart-target.md Outdated Show resolved Hide resolved
@parrt
Copy link
Member

parrt commented Oct 11, 2021

thanks guys!

@parrt parrt merged commit 00754cd into antlr:master Oct 11, 2021
@parrt parrt added this to the 4.9.3 milestone Oct 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants