Lint and fix `bugprone-use-after-move` violations by dnfield · Pull Request #35978 · flutter/engine

dnfield · 2022-09-07T22:42:24Z

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

gaaclarke

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.

gaaclarke · 2022-09-07T22:46:09Z

display_list/display_list_builder.cc

-      ? 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);


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

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

Ahh missed that, nice catch. LGTM.

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

gaaclarke · 2022-09-07T22:47:04Z

impeller/aiks/canvas.cc


-  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));


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

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

dnfield · 2022-09-07T22:52:05Z

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

auto-submit · 2022-09-08T00:58:58Z

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 · 2022-09-08T16:09:10Z

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 · 2022-09-08T16:14:14Z

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.

auto-submit · 2022-09-08T17:36:14Z

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 · 2022-09-08T20:21:52Z

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

…gine#35978)

)" This reverts commit 6279c78.

Start fixing bugprone-use-after-move violations

bd8dd8e

dnfield requested review from chinmaygarde, gaaclarke and jason-simmons September 7, 2022 22:42

flutter-dashboard bot added the platform-android label Sep 7, 2022

gaaclarke reviewed Sep 7, 2022

View reviewed changes

jason-simmons approved these changes Sep 7, 2022

View reviewed changes

gaaclarke approved these changes Sep 7, 2022

View reviewed changes

dnfield mentioned this pull request Sep 7, 2022

Use the frame size to cull #35973

Merged

dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 7, 2022

dnfield changed the title ~~Lint and fix bugprone-use-after-move violations~~ Lint and fix bugprone-use-after-move violations Sep 7, 2022

dnfield added 3 commits September 7, 2022 16:06

one missing NOLINT for a test

1e87006

another nolint for test

f36a325

Merge remote-tracking branch 'upstream/main' into tidy

277c382

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 8, 2022

dnfield added 3 commits September 7, 2022 21:53

one more

c306af7

one more...

1e0e7d9

Merge remote-tracking branch 'upstream/main' into tidy

3757595

dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 8, 2022

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 8, 2022

MORE!!

c27b168

dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 8, 2022

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 8, 2022

dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 9, 2022

auto-submit bot merged commit 6279c78 into flutter:main Sep 9, 2022

dnfield deleted the tidy branch September 9, 2022 02:57

engine-flutter-autoroll mentioned this pull request Sep 9, 2022

Roll Flutter Engine from 4096e133ef51 to 004bfba02487 (10 revisions) flutter/flutter#111238

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 9, 2022

6279c78 Lint and fix bugprone-use-after-move violations (flutter/en…

e4e2c3e

…gine#35978)

engine-flutter-autoroll mentioned this pull request Sep 9, 2022

Roll Flutter Engine from 4096e133ef51 to 6610f3f2a9fb (11 revisions) flutter/flutter#111240

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 9, 2022

6279c78 Lint and fix bugprone-use-after-move violations (flutter/en…

a664970

…gine#35978)

XilaiZhang added a commit to XilaiZhang/engine that referenced this pull request Sep 9, 2022

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

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

Lint and fix bugprone-use-after-move violations (flutter#35978)

04865a9

Conversation

dnfield commented Sep 7, 2022

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

gaaclarke Sep 7, 2022

Choose a reason for hiding this comment

Uh oh!

dnfield Sep 7, 2022

Choose a reason for hiding this comment

Uh oh!

gaaclarke Sep 7, 2022

Choose a reason for hiding this comment

Uh oh!

dnfield Sep 7, 2022

Choose a reason for hiding this comment

Uh oh!

gaaclarke Sep 7, 2022

Choose a reason for hiding this comment

Uh oh!

dnfield Sep 7, 2022

Choose a reason for hiding this comment

Uh oh!

dnfield commented Sep 7, 2022

Uh oh!

auto-submit bot commented Sep 8, 2022

Uh oh!

auto-submit bot commented Sep 8, 2022

Uh oh!

dnfield commented Sep 8, 2022

Uh oh!

auto-submit bot commented Sep 8, 2022

Uh oh!

zanderso commented Sep 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants