Skip to content

Conversation

@minaminao
Copy link
Contributor

In #12310 , RedundantAssignEliminator was renamed to UnusedAssignEliminator , but the documentation wasn't updated, so I fixed it.
FYI, the current implementation is here: https://github.com/ethereum/solidity/blob/29041c8101d13a5e4372e9c2b94e83a5e254d66e/libyul/optimiser/Suite.cpp#L313

Also, fixed several typos.

@github-actions
Copy link

Thank you for your contribution to the Solidity compiler! A team member will follow up shortly.

If you haven't read our contributing guidelines and our review checklist before, please do it now, this makes the reviewing process and accepting your contribution smoother.

If you have any questions or need our help, feel free to post them in the PR or talk to us directly on the #solidity-dev channel on Matrix.

Copy link
Member

@r0qs r0qs left a comment

Choose a reason for hiding this comment

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

Thanks @minaminao, just have some comments :)

.. code-block:: bash
docker run ethereum/solc:0.5.4 --help
docker run ethereum/solc:0.8.21 --help
Copy link
Member

Choose a reason for hiding this comment

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

The comment in the paragraph above and code-block in question with the version 0.5.4 was only meant as an example of a specific docker image tag. I believe we don't plan to keep bumping this information on every release, so I don't really see need to update it. But I'm fine with the change if you think it makes it more clear. The typo For example, You... should be fixed indeed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe we don't plan to keep bumping this information on every release

Yes, I agree with you! But do you think the version 0.5.4 is a too old?

Btw, I saw the following tweet about the Vyper bug.

https://twitter.com/WuBlockchain/status/1685827572033363968

Vyper official documentation recommendation is actually a faulty version. A bug in the smart contract language layer means that almost all protocols using Vyper are affected. Fortunately most protocols such as Uniswap use Solidity instead of the less popular Vyper. According to Slowmist

This tweet is wrong. It's clear from the Vyper's documentation that the 0.2.15 version is just an example version, and they don't recommend the 0.2.15 version.
But when I see this kind of misunderstanding, I think the version should be updated once in a while.

Copy link
Member

Choose a reason for hiding this comment

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

I believe we don't plan to keep bumping this information on every release

Yes, I agree with you! But do you think the version 0.5.4 is a too old?

Yes it is, but as it is mean as an example, I don't really expect that anyone would think that we are recommending such version, because we are not. However, I'm fine with the change, but this version will become obsolete in the next release anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Like @r0qs said, this was not meant to be kept up to date and will go out of date again soon. But you do have a point. Some users could copy it without making sure it's a recent version, especially if they're new to Solidity and don't know that 0.5.x is ancient by now. We should replace it with a tag pointing at the latest version and just mention in the surrounding text that tags for specific versions are available too. Then an out of date version would not be such a problem because people would get the latest one by default.

Suggested change
docker run ethereum/solc:0.8.21 --help
docker run ethereum/solc:stable --help

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, this currently sits in a commit described as fix: typo. Something like this should really get a separate commit.

Copy link
Contributor Author

@minaminao minaminao Aug 17, 2023

Choose a reason for hiding this comment

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

@cameel Thanks for your comment.
The current documentation already describes a command using stable as follows, and changing the specific version to stable seems unnatural to me.
image

As an alternative, I think using 0.x.y is good: docker run ethereum/solc:0.x.y --help.
0.x.y is used at the beginning of this page and on other pages, so users can understand it.

image

You're right about the commit issue, so I split it into separate commits!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, true :) I did not notice that we already have stable above.

In that case I don't think this bit with a specific version is even needed. I mean, with a concrete version it did make some sense, but if you replace it with x.y.z, I think we're better off only mentioning that as a part of the surrounding text like I suggested.

BTW, I'd use x.y.z instead of 0.x.z. That bit you screenshotted specifically refers to major release 0 so it's relevant there, but in the rest of the docs, we don't have to assume we're at 0. The number will go up some day and the less we'll have to update then the better :)

Copy link
Member

Choose a reason for hiding this comment

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

In that case I don't think this bit with a specific version is even needed. I mean, with a concrete version it did make some sense, but if you replace it with x.y.z, I think we're better off only mentioning that as a part of the surrounding text like I suggested.

Yeah, maybe it is better indeed to do what @cameel suggested and only mention the use with tags for specific version in the surrounding text, removing it from the example. @minaminao if you do that, we can already merge it :)

@minaminao
Copy link
Contributor Author

@r0qs
used colons and squashed the commit!

r0qs
r0qs previously approved these changes Aug 2, 2023
@cameel cameel changed the title docs: rename RedundantAssignEliminator to UnusedAssignEliminator docs: rename RedundantAssignEliminator to UnusedAssignEliminator + other small corrections Aug 16, 2023
cameel
cameel previously approved these changes Aug 16, 2023
Copy link
Collaborator

@cameel cameel left a comment

Choose a reason for hiding this comment

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

Thanks for the corrections!

I'd be fine with merging it already, but it would be best to change the docker tag to stable first as I suggested in comments below.

.. code-block:: bash
docker run ethereum/solc:0.5.4 --help
docker run ethereum/solc:0.8.21 --help
Copy link
Collaborator

Choose a reason for hiding this comment

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

Like @r0qs said, this was not meant to be kept up to date and will go out of date again soon. But you do have a point. Some users could copy it without making sure it's a recent version, especially if they're new to Solidity and don't know that 0.5.x is ancient by now. We should replace it with a tag pointing at the latest version and just mention in the surrounding text that tags for specific versions are available too. Then an out of date version would not be such a problem because people would get the latest one by default.

Suggested change
docker run ethereum/solc:0.8.21 --help
docker run ethereum/solc:stable --help

.. code-block:: bash
docker run ethereum/solc:0.5.4 --help
docker run ethereum/solc:0.8.21 --help
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, this currently sits in a commit described as fix: typo. Something like this should really get a separate commit.

@mehtavishwa30
Copy link
Contributor

Thank you for your contribution. I will make a small change to accommodate @cameel's comment regarding changing the docker tag to stable and clarifying that version tags are available in surrounding text. This is to ensure users get the latest version by default and avoid an out-of-date version.

Will commit the change and merge once all checks pass. :)

@mehtavishwa30 mehtavishwa30 added the takeover Can be taken over by someone other than label giver label Mar 1, 2024
@mehtavishwa30 mehtavishwa30 requested a review from r0qs March 2, 2024 16:53
Copy link
Member

@r0qs r0qs left a comment

Choose a reason for hiding this comment

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

Looks good to me. Just one last comment ;)

nikola-matic
nikola-matic previously approved these changes Mar 4, 2024
Co-authored-by: r0qs <rodrigo.saramago@protonmail.com>
@mehtavishwa30 mehtavishwa30 merged commit 8f0cb8a into argotorg:develop Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation 📖 external contribution ⭐ takeover Can be taken over by someone other than label giver

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants