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

Jump to bottom

terror: remove dead code which may cause data race #1096

Merged
merged 4 commits into from Nov 23, 2020
Merged

terror: remove dead code which may cause data race #1096

merged 4 commits into from Nov 23, 2020

Conversation

blacktear23
Copy link
Contributor

@blacktear23 blacktear23 commented Nov 15, 2020 •
edited

What problem does this PR solve?

close issues:

What is changed and how it works?

remove useless code and this can cause data race.

Check List

Tests

  • No code

Code changes

  • No

Side effects

  • No

Related changes

  • No

@blacktear23 blacktear23 changed the title terror: remove dead code terror: remove dead code which may cause data race Nov 15, 2020
@blacktear23
Copy link
Contributor Author

/run-all-tests

kennytm
kennytm reviewed Nov 16, 2020
View reviewed changes
Copy link
Contributor

@kennytm kennytm left a comment

Choose a reason for hiding this comment

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

LGTM, though IMO you could cut the code2ErrClassMap type and use that sync.Map with Load and Store directly, they are private implementation details anyway 🤷.

@kennytm kennytm added the status/LGT1 LGT1 label Nov 16, 2020
@blacktear23
Copy link
Contributor Author

LGTM, though IMO you could cut the code2ErrClassMap type and use that sync.Map with Load and Store directly, they are private implementation details anyway 🤷.

For now, golang not support generic, so I prefer to use this style to write less typecast code.😁

tangenta
tangenta approved these changes Nov 23, 2020
View reviewed changes
Copy link
Contributor

@tangenta tangenta left a comment

Choose a reason for hiding this comment

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

LGTM

@kennytm kennytm added status/LGT2 LGT2 and removed status/LGT1 LGT1 labels Nov 23, 2020
@kennytm
Copy link
Contributor

kennytm commented Nov 23, 2020

/merge

@ti-srebot
Copy link
Contributor

@kennytm Oops! This PR requires at least 2 LGTMs to merge. The current number of LGTM is 1

@kennytm
Copy link
Contributor

kennytm commented Nov 23, 2020

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@blacktear23 merge failed.

@kennytm kennytm merged commit bfa7abb into pingcap:master Nov 23, 2020
tiancaiamao pushed a commit to tiancaiamao/parser that referenced this pull request Apr 27, 2021
@blacktear23
terror: remove dead code which may cause data race (pingcap#1096)
cb4fac9 
* terror: remove dead code

* terror: fix potential data race
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kennytm kennytm kennytm approved these changes

@tangenta tangenta tangenta approved these changes

@ti-srebot ti-srebot ti-srebot approved these changes

Assignees
No one assigned
Labels
status/can-merge status/LGT2 LGT2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@blacktear23 @kennytm @ti-srebot @tangenta