[go: nahoru, domu]
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.

Sign up for GitHub

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

Lint and fix bugprone-use-after-move violations #35978

Merged
merged 8 commits into from
Sep 9, 2022
Merged

Lint and fix bugprone-use-after-move violations #35978

merged 8 commits into from
Sep 9, 2022

Conversation

dnfield
Copy link
Contributor
@dnfield dnfield commented Sep 7, 2022

Fixes flutter/flutter#111135

Adds some ignores for tests that explicitly want to test behavior of use after move.

gaaclarke
gaaclarke reviewed Sep 7, 2022
View reviewed changes
Copy link
Member
@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

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

Awesome, LGTM. I thought we had this on already. I remember fixing the issue when I first implemented the linter. My only concern was its inability to figure out the ternary operator case. I don't want to accept worse performance to make the linter happy.

display_list/display_list_builder.cc
Comment on lines -1044 to -1046
? Push<DrawSkPictureMatrixOp>(0, 1, std::move(picture), *matrix,
? Push<DrawSkPictureMatrixOp>(0, 1, picture, *matrix,
render_with_attributes)
: Push<DrawSkPictureOp>(0, 1, std::move(picture), render_with_attributes);
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a false alarm. Only one move is guaranteed to execute and this technically changes the performance for the worse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem isn't the ternary, it's that we reference picture on the line right under this.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh missed that, nice catch. LGTM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Well, the line under the comments under this, which GitHub is cutting off by default :)

impeller/aiks/canvas.cc
Comment on lines -339 to 342
lazy_glyph_atlas->AddTextFrame(std::move(text_frame));
lazy_glyph_atlas->AddTextFrame(text_frame);

auto text_contents = std::make_shared<TextContents>();
text_contents->SetTextFrame(std::move(text_frame));
Copy link
Member

Choose a reason for hiding this comment

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

How is this not a bug? This manifests as a bug right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chinmaygarde may know. I don't understand how this ever worked :)

@dnfield
Copy link
Contributor Author
dnfield commented Sep 7, 2022

Awesome, LGTM. I thought we had this on already. I remember fixing the issue when I first implemented the linter. My only concern was its inability to figure out the ternary operator case. I don't want to accept worse performance to make the linter happy.

There's some lint in there that is checking for calling methods on std::moved objects, but it doesn't appear to cover as many cases of use after move. bugprone-use-after-move is covering more. I thought of this because I was working on a patch where I was worried I might have introduced a use-after-move and started wondering if the linter was set up to catch those...

@dnfield dnfield mentioned this pull request Sep 7, 2022
Merged
@dnfield dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 7, 2022
@dnfield dnfield changed the title Lint and fix bugprone-use-after-move violations Lint and fix bugprone-use-after-move violations Sep 7, 2022
@auto-submit
Copy link
Contributor
auto-submit bot commented Sep 8, 2022

auto label is removed for flutter/engine, pr: 35978, due to - The status or check suite Mac clang-tidy has failed. Please fix the issues identified (or deflake) before re-applying this label.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 8, 2022
@dnfield dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 8, 2022
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 8, 2022
@auto-submit
Copy link
Contributor
auto-submit bot commented Sep 8, 2022

auto label is removed for flutter/engine, pr: 35978, due to - The status or check suite Linux clang-tidy has failed. Please fix the issues identified (or deflake) before re-applying this label.

@dnfield
Copy link
Contributor Author
dnfield commented Sep 8, 2022

There have been a few violations introduced by new commits as I'm writing this PR. If it happens to fail in post submit we should fix forward rather than reverting.

The main culprit seems to be people copy/pasting something like CreateShell(std::move(settings), std::move(task_runners)) from buggy tests that are getting fixed here.

@dnfield dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 8, 2022
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 8, 2022
@auto-submit
Copy link
Contributor
auto-submit bot commented Sep 8, 2022

auto label is removed for flutter/engine, pr: 35978, due to - The status or check suite Mac iOS Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

@zanderso
Copy link
Member
zanderso commented Sep 8, 2022

From PR triage: This is blocked on the tree closure @ flutter/flutter#111193.

@dnfield dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 9, 2022
@auto-submit auto-submit bot merged commit 6279c78 into flutter:main Sep 9, 2022
@dnfield dnfield deleted the tidy branch September 9, 2022 02:57
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Sep 9, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 9, 2022
@engine-flutter-autoroll
6279c789b Lint and fix bugprone-use-after-move violations (flutter/…
e4e2c3e 
…engine#35978)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Sep 9, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 9, 2022
@engine-flutter-autoroll
6279c789b Lint and fix bugprone-use-after-move violations (flutter/…
a664970 
…engine#35978)
XilaiZhang added a commit to XilaiZhang/engine that referenced this pull request Sep 9, 2022
@XilaiZhang
Revert "Lint and fix bugprone-use-after-move violations (flutter#35978
fcdcb4e 
)"

This reverts commit 6279c78.
cfontas pushed a commit to cfontas/engine that referenced this pull request Sep 14, 2022
@dnfield @cfontas
Lint and fix bugprone-use-after-move violations (flutter#35978)
46b663a
Oleh-Sv pushed a commit to Oleh-Sv/engine that referenced this pull request Sep 28, 2022
@dnfield @OlehSv
Lint and fix bugprone-use-after-move violations (flutter#35978)
04865a9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jason-simmons jason-simmons jason-simmons approved these changes

@gaaclarke gaaclarke gaaclarke approved these changes

@chinmaygarde chinmaygarde Awaiting requested review from chinmaygarde

Assignees
No one assigned
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-android
Projects
None yet
Milestone
No milestone
4 participants
@dnfield @zanderso @jason-simmons @gaaclarke