Disallow copy and move #4578

ZHANGWENTAI · 2022-04-02T16:14:38Z

What problem does this PR solve?

Issue Number: ref #4411

Problem Summary: use DISALLOW_COPY_AND_MOVE instead.

What is changed and how it works?

add DISALLOW_COPY, DISALLOW_MOVE, DISALLOWCOPY_AND_MOVE macro, and used in the files listed in #4411.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

None

ti-chi-bot · 2022-04-02T16:14:39Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

Lloyd-Pottiger
ywqzzy

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

CLAassistant · 2022-04-02T16:14:44Z

All committers have signed the CLA.

ywqzzy · 2022-04-06T03:30:01Z

dbms/src/Common/CPUAffinityManager.h

+    DISALLOW_COPY(ClassName);             \
+    DISALLOW_MOVE(ClassName)
+
+#define DISALLOW_COPY(ClassName)           \


You can write the code in /dbms/src/Common/nocopyable.h

I have done as you said. And I wonder if I need to replace the one line deletion of copy constructor or assign constructor like AggregateDescription(const AggregateDescription &) = delete; in /dbms/src/DataStreams/SummingSortedBlockInputStream.h, is it on purpose or I can replace it with DISALLOW_COPY(AggregateDescription) directly?

You can replace it directly. Maybe you can list some of the classes need to refactor in the issue and create some sub-issue, and you refactor 3 or 4 of them in one pr.

Like @ywqzzy said, the macro DISALLOW_COPY/DISALLOW_MOVE /DISALLOW_COPY_AND_MOVE should be defined in a common place like dbms/src/Common/noncopyable.h.

And I wonder whether we can define DISALLOW_COPY_AND_MOVE before DISALLOW_COPY/DISALLOW_MOVE ?

fuzhe1989 · 2022-04-07T14:28:48Z

@JaySon-Huang could you help to review this pr?

ywqzzy · 2022-04-14T04:10:51Z

/rebuild

ZHANGWENTAI · 2022-04-14T13:26:07Z

/rebuild

ZHANGWENTAI · 2022-04-21T08:12:07Z

I found a mistake in dbms/src/Common/ExecutableTask.h, let me fix it and recommit.

ZHANGWENTAI · 2022-04-28T07:30:37Z

Can anyone review this pr? thx :)

ywqzzy · 2022-04-28T12:57:56Z

/run-all-tests

ywqzzy

LGTM. Good Job!

sre-bot · 2022-04-28T13:27:50Z

Coverage for changed files

too many lines from llvm-cov, please refer to full report instead

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18053      10149            43.78%    200107  100945       49.55%

full coverage report (for internal network access only)

Lloyd-Pottiger

Thanks for your good job! The two copy operations are independent. Declaring copy constructor does not prevent compiler to generate copy assignment and vice versa. And I found that there are some classes which only make copy constructor = delete but you also replace it with DISALLOW_COPY(className). In most cases, it is fine. However, could you please take a deep check to make sure it is all fine? And thanks for your great contribution again!

Lloyd-Pottiger · 2022-04-29T02:03:17Z

dbms/src/Storages/Page/V3/PageDirectory.h

@@ -359,11 +360,8 @@ class PageDirectory
    }

    // No copying
-    PageDirectory(const PageDirectory &) = delete;
-    PageDirectory & operator=(const PageDirectory &) = delete;
    // No moving


maybe combine the two comments?

Lloyd-Pottiger · 2022-04-29T02:18:37Z

And why do this? 😂

- std::string toString() const { return "Not Support"; }
+ std::string toString() const
+ {
+      return "Not Support";
+  }

honestly, I like the previous code style which makes code more compact with high readability. 😄

Signed-off-by: ZHANGWENTAI <[email protected]>

ZHANGWENTAI · 2022-05-06T07:03:57Z

Maybe the formatter automatically did the change.

we have enable AllowShortFunctionsOnASingleLine in .clang-format

I format the code in my local repo by python3 format-diff.py --diff_from git merge-base master HEAD``, but it still expend short functions. Is I doing it right?

Yes. AllowShortFunctionsOnASingleLine: Inline in clang-format means allow not force. Therefore, your style is legal for clang-format, so there is nothing changed 😂

hello! I have remained the format of short function and commit agian

Lloyd-Pottiger · 2022-05-06T07:42:50Z

Maybe the formatter automatically did the change.

we have enable AllowShortFunctionsOnASingleLine in .clang-format

I format the code in my local repo by python3 format-diff.py --diff_from git merge-base master HEAD``, but it still expend short functions. Is I doing it right?

Yes. AllowShortFunctionsOnASingleLine: Inline in clang-format means allow not force. Therefore, your style is legal for clang-format, so there is nothing changed 😂

hello! I have remained the format of short function and commit agian

https://github.com/pingcap/tiflash/pull/4578/files#diff-e49f478fa33ca6648a69a7aba47d9485827d89793ef5784c080375ff2fce5cfb seems have not pushed?

ZHANGWENTAI · 2022-05-06T08:39:32Z

Maybe the formatter automatically did the change.

we have enable AllowShortFunctionsOnASingleLine in .clang-format

I format the code in my local repo by python3 format-diff.py --diff_from git merge-base master HEAD``, but it still expend short functions. Is I doing it right?

Yes. AllowShortFunctionsOnASingleLine: Inline in clang-format means allow not force. Therefore, your style is legal for clang-format, so there is nothing changed 😂

hello! I have remained the format of short function and commit agian

https://github.com/pingcap/tiflash/pull/4578/files#diff-e49f478fa33ca6648a69a7aba47d9485827d89793ef5784c080375ff2fce5cfb seems have not pushed?

Actually I did 😂. The reason why I expand those functions still is to keep code style consistent in the file and some functions are not short enough. Expanding them is easier to read

Lloyd-Pottiger · 2022-05-06T09:29:04Z

Maybe the formatter automatically did the change.

we have enable AllowShortFunctionsOnASingleLine in .clang-format

I format the code in my local repo by python3 format-diff.py --diff_from git merge-base master HEAD``, but it still expend short functions. Is I doing it right?

Yes. AllowShortFunctionsOnASingleLine: Inline in clang-format means allow not force. Therefore, your style is legal for clang-format, so there is nothing changed 😂

hello! I have remained the format of short function and commit agian

https://github.com/pingcap/tiflash/pull/4578/files#diff-e49f478fa33ca6648a69a7aba47d9485827d89793ef5784c080375ff2fce5cfb seems have not pushed?

Actually I did 😂. The reason why I expand those functions still is to keep code style consistent in the file and some functions are not short enough. Expanding them is easier to read

OK，cool

Lloyd-Pottiger · 2022-05-06T10:13:20Z

@ywqzzy PTAL

ywqzzy · 2022-05-06T12:39:03Z

/run-all-tests

sre-bot · 2022-05-06T13:08:20Z

Coverage for changed files

too many lines from llvm-cov, please refer to full report instead

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18077      10153            43.83%    200562  101039       49.62%

full coverage report (for internal network access only)

ywqzzy · 2022-05-07T04:20:13Z

/merge

ti-chi-bot · 2022-05-07T04:20:14Z

@ywqzzy: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2022-05-07T04:20:16Z

This pull request has been accepted and is ready to merge.

Commit hash: b1f5f25

sre-bot · 2022-05-07T04:54:07Z

Coverage for changed files

too many lines from llvm-cov, please refer to full report instead

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18090      10150            43.89%    200902  100984       49.73%

full coverage report (for internal network access only)

ZHANGWENTAI · 2022-05-07T05:47:17Z

PTAL

sre-bot · 2022-05-07T10:54:24Z

Coverage for changed files

too many lines from llvm-cov, please refer to full report instead

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18090      10151            43.89%    200906  101002       49.73%

full coverage report (for internal network access only)

ti-chi-bot · 2022-05-07T11:20:43Z

@ZHANGWENTAI: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

sre-bot · 2022-05-07T11:47:33Z

Coverage for changed files

too many lines from llvm-cov, please refer to full report instead

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18091      10150            43.89%    200958  100975       49.75%

full coverage report (for internal network access only)

ti-chi-bot added the release-note-none Denotes a PR that doesn't merit a release note. label Apr 2, 2022

ti-chi-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 2, 2022

ywqzzy reviewed Apr 6, 2022

View reviewed changes

ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 13, 2022

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 18, 2022

ZHANGWENTAI force-pushed the disallow_copy_and_move branch from deae845 to 6c60aff Compare April 21, 2022 03:32

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 21, 2022

ZHANGWENTAI force-pushed the disallow_copy_and_move branch 2 times, most recently from 3ada1d7 to 09ad8b5 Compare April 21, 2022 04:44

ywqzzy self-requested a review April 21, 2022 07:15

ZHANGWENTAI force-pushed the disallow_copy_and_move branch 3 times, most recently from f4c6f44 to 70e00ee Compare April 26, 2022 02:54

ywqzzy requested a review from JaySon-Huang April 28, 2022 12:53

ywqzzy requested a review from Lloyd-Pottiger April 28, 2022 12:58

ywqzzy approved these changes Apr 28, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 28, 2022

ywqzzy self-requested a review April 28, 2022 15:37

Lloyd-Pottiger reviewed Apr 29, 2022

View reviewed changes

add macros to disallow copy or move, and refactor some classes

2929380

Signed-off-by: ZHANGWENTAI <[email protected]>

ZHANGWENTAI force-pushed the disallow_copy_and_move branch from 10506f4 to 2929380 Compare May 5, 2022 06:07

Lloyd-Pottiger approved these changes May 6, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels May 6, 2022

Merge branch 'master' into disallow_copy_and_move

67c18a9

Merge branch 'master' into disallow_copy_and_move

b1f5f25

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label May 7, 2022

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 7, 2022

Merge branch 'master' into disallow_copy_and_move

1a5489c

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 7, 2022

Merge branch 'master' into disallow_copy_and_move

7adfb26

Merge branch 'master' into disallow_copy_and_move

10770b0

ti-chi-bot merged commit 7ffb943 into pingcap:master May 7, 2022

ZHANGWENTAI deleted the disallow_copy_and_move branch May 7, 2022 12:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disallow copy and move #4578

Disallow copy and move #4578

ZHANGWENTAI commented Apr 2, 2022 •

edited

Loading

ti-chi-bot commented Apr 2, 2022 •

edited

Loading

CLAassistant commented Apr 2, 2022 •

edited

Loading

ywqzzy Apr 6, 2022

ZHANGWENTAI Apr 7, 2022 •

edited

Loading

ywqzzy Apr 11, 2022

JaySon-Huang Apr 11, 2022

fuzhe1989 commented Apr 7, 2022

ywqzzy commented Apr 14, 2022

ZHANGWENTAI commented Apr 14, 2022

ZHANGWENTAI commented Apr 21, 2022

ZHANGWENTAI commented Apr 28, 2022

ywqzzy commented Apr 28, 2022

ywqzzy left a comment

sre-bot commented Apr 28, 2022

Lloyd-Pottiger left a comment

Lloyd-Pottiger Apr 29, 2022

Lloyd-Pottiger commented Apr 29, 2022

ZHANGWENTAI commented May 6, 2022

Lloyd-Pottiger commented May 6, 2022

ZHANGWENTAI commented May 6, 2022 •

edited

Loading

Lloyd-Pottiger commented May 6, 2022

Lloyd-Pottiger commented May 6, 2022

ywqzzy commented May 6, 2022

sre-bot commented May 6, 2022

ywqzzy commented May 7, 2022

ti-chi-bot commented May 7, 2022

ti-chi-bot commented May 7, 2022

sre-bot commented May 7, 2022

ZHANGWENTAI commented May 7, 2022

sre-bot commented May 7, 2022

ti-chi-bot commented May 7, 2022

sre-bot commented May 7, 2022

Disallow copy and move #4578

Disallow copy and move #4578

Conversation

ZHANGWENTAI commented Apr 2, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Apr 2, 2022 • edited Loading

CLAassistant commented Apr 2, 2022 • edited Loading

ywqzzy Apr 6, 2022

Choose a reason for hiding this comment

ZHANGWENTAI Apr 7, 2022 • edited Loading

Choose a reason for hiding this comment

ywqzzy Apr 11, 2022

Choose a reason for hiding this comment

JaySon-Huang Apr 11, 2022

Choose a reason for hiding this comment

fuzhe1989 commented Apr 7, 2022

ywqzzy commented Apr 14, 2022

ZHANGWENTAI commented Apr 14, 2022

ZHANGWENTAI commented Apr 21, 2022

ZHANGWENTAI commented Apr 28, 2022

ywqzzy commented Apr 28, 2022

ywqzzy left a comment

Choose a reason for hiding this comment

sre-bot commented Apr 28, 2022

Coverage for changed files

Coverage summary

Lloyd-Pottiger left a comment

Choose a reason for hiding this comment

Lloyd-Pottiger Apr 29, 2022

Choose a reason for hiding this comment

Lloyd-Pottiger commented Apr 29, 2022

ZHANGWENTAI commented May 6, 2022

Lloyd-Pottiger commented May 6, 2022

ZHANGWENTAI commented May 6, 2022 • edited Loading

Lloyd-Pottiger commented May 6, 2022

Lloyd-Pottiger commented May 6, 2022

ywqzzy commented May 6, 2022

sre-bot commented May 6, 2022

Coverage for changed files

Coverage summary

ywqzzy commented May 7, 2022

ti-chi-bot commented May 7, 2022

ti-chi-bot commented May 7, 2022

sre-bot commented May 7, 2022

Coverage for changed files

Coverage summary

ZHANGWENTAI commented May 7, 2022

sre-bot commented May 7, 2022

Coverage for changed files

Coverage summary

ti-chi-bot commented May 7, 2022

sre-bot commented May 7, 2022

Coverage for changed files

Coverage summary

ZHANGWENTAI commented Apr 2, 2022 •

edited

Loading

ti-chi-bot commented Apr 2, 2022 •

edited

Loading

CLAassistant commented Apr 2, 2022 •

edited

Loading

ZHANGWENTAI Apr 7, 2022 •

edited

Loading

ZHANGWENTAI commented May 6, 2022 •

edited

Loading