Configure dart debug flags through environment declaration by canastro · Pull Request #3128 · antlr/antlr4

@canastro

Issue: #3127

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

@canastro

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

lingyv-li

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

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

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

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

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

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.

lingyv-li

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

lingyv-li

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

@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

@lingyv-li

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

ericvergnaud

@canastro

@parrt