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

cmd/evm: rename t8n args to improve clarity when tracing #23934

Merged
merged 3 commits into from Nov 24, 2021

Conversation

lightclient
Copy link
Member

Apologizes for another PR - my new commits weren't coming through in #23933.

Okay I misunderstood the meaning of the args tracer.nomemory and tracer.noreturndata. I understand now that it is preferred that memory and return data are not printed by default. Given this, I think the flag name are confusing. I didn't realize until looking deeply into this that --tracer.nomemory=false is a valid argument. I would've assumed the flag did not take a value and was a simple on/off switch, and therefore assumed the correct default behavior for these values should be on. Others have also encountered this.

For these reasons, I propose renaming them to tracer.memory and tracer.returndata to avoid the double-negative flag tracer.nomemory=false.

@holiman
Copy link
Contributor

holiman commented Nov 22, 2021

Yeah, this is obviously the preferred approach, and we would have done that if we weren't so averse to making API changes. The "proper" way to do this is to have both flags in parallel for a while, and print an error message when the deprecated (nomemory) flag is used.

@lightclient
Copy link
Member Author

Good point @holiman. I added the flags back and marked them as deprecated. I also decided to return an error if the flags are used in a conflicting manner. Not sure you handle situations in the past, but I can remove if you prefer.

@holiman
Copy link
Contributor

holiman commented Nov 23, 2021

Please add a printout whenever the deprecated flag is used, telling the user to use the new format instead.

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM

@holiman holiman added this to the 1.10.13 milestone Nov 24, 2021
@holiman holiman merged commit 0a7672f into ethereum:master Nov 24, 2021
sidhujag pushed a commit to syscoin/go-ethereum that referenced this pull request Nov 24, 2021
)

* cmd/evm: rename t8n args to improve clarity when tracing

* cmd/evm: add back removed tracing flags and note that they are deprecated

* cmd/evm: add warning when using deprecated flag
Copy link

@WEEDAVIE1990 WEEDAVIE1990 left a comment

Choose a reason for hiding this comment

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

cmd/evm/main.go

yongjun925 pushed a commit to DODOEX/go-ethereum that referenced this pull request Dec 3, 2022
)

* cmd/evm: rename t8n args to improve clarity when tracing

* cmd/evm: add back removed tracing flags and note that they are deprecated

* cmd/evm: add warning when using deprecated flag
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.

None yet

4 participants