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

Prevent NameSimplifier and VarNameCleaner from producing Yul identifiers with leading and trailing dots #15149

Merged

Conversation

clonker
Copy link
Member

@clonker clonker commented May 27, 2024

Fixes #11496

@clonker clonker requested review from cameel and nikola-matic May 27, 2024 10:40
Copy link
Member

@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.

Also some general remarks:

  • The PR is missing a changelog entry.
  • When a PR fixes everything in the original issue you should use the github feature that will fix the issue automatically when the PR is merged. I.e. just add Fixes #11496 in the PR description.
  • I saw that the issue was moved to the 'In progress' column on the triage board. That column actually means that triage is progress :) It was added so that multiple people don't waste time triaging the same issue at the same time. The issues are only really meant to spent a short time there - triage never really takes all that long.

@cameel cameel changed the title Make yul identifies with leading and trailing dots restricted Make yul identifiers with leading and trailing dots restricted May 27, 2024
@cameel cameel changed the title Make yul identifiers with leading and trailing dots restricted Prevent NameSimplifier and VarNameCleaner from producing Yul identifiers with leading and trailing dots May 27, 2024
@cameel
Copy link
Member

cameel commented May 27, 2024

  • When a PR fixes everything in the original issue you should use the github feature that will fix the issue automatically when the PR is merged. I.e. just add Fixes #11496 in the PR description.

Just noticed that you do have this but it's in the commit message. I'd recommend against it since this adds a lot of noise to the issue you mention this way - the commit appears there again and again each time you rebase the branch. It works much better when used in the PR instead.

@clonker clonker force-pushed the yul_leading_trailing_dot_in_identifier branch 3 times, most recently from f96d849 to d4c4b06 Compare May 28, 2024 13:24
@clonker
Copy link
Member Author

clonker commented May 28, 2024

Also some general remarks:

  • The PR is missing a changelog entry.
  • When a PR fixes everything in the original issue you should use the github feature that will fix the issue automatically when the PR is merged. I.e. just add Fixes #11496 in the PR description.
  • I saw that the issue was moved to the 'In progress' column on the triage board. That column actually means that triage is progress :) It was added so that multiple people don't waste time triaging the same issue at the same time. The issues are only really meant to spent a short time there - triage never really takes all that long.

Ahh I have misunderstood the "in progress" :) thanks for pointing that out!

@clonker clonker force-pushed the yul_leading_trailing_dot_in_identifier branch 2 times, most recently from d5aadd0 to 947198b Compare May 28, 2024 15:29
nikola-matic
nikola-matic previously approved these changes May 28, 2024
test/libyul/objectCompiler/leading_and_trailing_dots.yul Outdated Show resolved Hide resolved
test/libyul/objectCompiler/leading_and_trailing_dots.yul Outdated Show resolved Hide resolved
Changelog.md Outdated Show resolved Hide resolved
@clonker clonker force-pushed the yul_leading_trailing_dot_in_identifier branch 2 times, most recently from 6c97388 to 7f89de1 Compare May 28, 2024 15:44
cameel
cameel previously approved these changes May 28, 2024
Copy link
Member

@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.

If it passes CI, it's good to go.

@clonker clonker force-pushed the yul_leading_trailing_dot_in_identifier branch from 7f89de1 to bbd5e2e Compare May 28, 2024 16:02
@clonker
Copy link
Member Author

clonker commented May 28, 2024

Whelp, forgot to update the test outcome after updating indentations, care to re-approve @cameel ?:)

@clonker clonker merged commit 15b99ca into ethereum:develop May 28, 2024
71 of 72 checks passed
@clonker clonker deleted the yul_leading_trailing_dot_in_identifier branch May 28, 2024 16:39
clonker added a commit that referenced this pull request Jun 3, 2024
nikola-matic added a commit that referenced this pull request Jun 3, 2024
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.

NameSimplifier sometimes produces invalid Yul code by leaving a trailing dot at the end of an identifier
3 participants