New issue
Advanced search Search tips

Issue 703565 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Cleanup "return std::move(foo);" pattern after Xcode's clang issue is fixed

Project Member Reported by sdefresne@chromium.org, Mar 21 2017

Issue description

A bug in the version of clang shipped with Xcode 8.x causes the following valid code to fail compilation:

struct Foo { virtual ~Foo() = default; };
struct Bar : public Foo { void Init(); };

std::unique_ptr<Foo> CreateFoo() {
  std::unique_ptr<Bar> bar(new Bar);
  bar->Init();
  return bar;
}

The workaround is to change the code to the following:

std::unique_ptr<Foo> CreateFoo() {
  std::unique_ptr<Bar> bar(new Bar);
  bar->Init();
  return std::move(bar);
}

However, it is discouraged to use an explicit "std::move()" for the returned value as it may cause the compiler to disable Return Value Optimisation (RVO). Remove that pattern once Chrome on iOS requires Xcode version with a fixed clang.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Mar 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e7744503e5d45e5404373b447e1499c981174eb9

commit e7744503e5d45e5404373b447e1499c981174eb9
Author: vabr <vabr@chromium.org>
Date: Tue Mar 21 12:49:01 2017

[password_manager] Add TODO to record usages of "return std::move(foo)" pattern.

Due to a bug in the version of clang shipped with Xcode 8.x (used
to build official), it is required to use sub-optimal construct
"return std::move(foo)" to return local variable foo if foo is a
unique pointer of a sub-class of the function prototype. Mark all
occurrences of the pattern to easily find them once Xcode bug is
fixed.

Following https://codereview.chromium.org/2762973002/ for iOS, this CL marks
the workaround patter in password manager code.

BUG= 703565 
R=sdefresne@chromium.org

Review-Url: https://codereview.chromium.org/2763973002
Cr-Commit-Position: refs/heads/master@{#458383}

[modify] https://crrev.com/e7744503e5d45e5404373b447e1499c981174eb9/components/password_manager/core/browser/form_fetcher_impl.cc

Components: Internals
Project Member

Comment 3 by bugdroid1@chromium.org, Sep 27 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/df9b10be95d37143a1523091a9c66c9439c87b43

commit df9b10be95d37143a1523091a9c66c9439c87b43
Author: Sylvain Defresne <sdefresne@chromium.org>
Date: Wed Sep 27 16:40:46 2017

Remove workaround for bug in Xcode's clang.

The version of clang shipped with Xcode up to 9.0 (excluded) had
a bug where returning a std::unique_ptr<Derived> from a function
declared as returning a std::unique_ptr<Base> failed.

As Xcode 9.0 or higher is required to compile Chrome on iOS, the
workaround (explicitly doing "return std::move(derived)") can be
removed.

Bug:  703565 
Change-Id: I1d04872ff071c8e536a18bee6724e66b45250e48
Reviewed-on: https://chromium-review.googlesource.com/687514
Reviewed-by: Elodie Banel <lod@chromium.org>
Commit-Queue: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504702}
[modify] https://crrev.com/df9b10be95d37143a1523091a9c66c9439c87b43/ios/chrome/browser/autocomplete/in_memory_url_index_factory.cc
[modify] https://crrev.com/df9b10be95d37143a1523091a9c66c9439c87b43/ios/chrome/browser/autofill/personal_data_manager_factory.cc
[modify] https://crrev.com/df9b10be95d37143a1523091a9c66c9439c87b43/ios/chrome/browser/bookmarks/bookmark_model_factory.cc
[modify] https://crrev.com/df9b10be95d37143a1523091a9c66c9439c87b43/ios/chrome/browser/browser_state/chrome_browser_state_io_data.cc
[modify] https://crrev.com/df9b10be95d37143a1523091a9c66c9439c87b43/ios/chrome/browser/browser_state/test_chrome_browser_state.mm
[modify] https://crrev.com/df9b10be95d37143a1523091a9c66c9439c87b43/ios/chrome/browser/history/history_service_factory.cc
[modify] https://crrev.com/df9b10be95d37143a1523091a9c66c9439c87b43/ios/chrome/browser/infobars/infobar_utils.mm
[modify] https://crrev.com/df9b10be95d37143a1523091a9c66c9439c87b43/ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory_util.cc
[modify] https://crrev.com/df9b10be95d37143a1523091a9c66c9439c87b43/ios/chrome/browser/reading_list/reading_list_download_service_factory.cc
[modify] https://crrev.com/df9b10be95d37143a1523091a9c66c9439c87b43/ios/chrome/browser/signin/about_signin_internals_factory.cc
[modify] https://crrev.com/df9b10be95d37143a1523091a9c66c9439c87b43/ios/chrome/browser/signin/account_fetcher_service_factory.cc
[modify] https://crrev.com/df9b10be95d37143a1523091a9c66c9439c87b43/ios/chrome/browser/signin/account_reconcilor_factory.cc
[modify] https://crrev.com/df9b10be95d37143a1523091a9c66c9439c87b43/ios/chrome/browser/signin/account_tracker_service_factory.cc
[modify] https://crrev.com/df9b10be95d37143a1523091a9c66c9439c87b43/ios/chrome/browser/signin/authentication_service_factory.mm
[modify] https://crrev.com/df9b10be95d37143a1523091a9c66c9439c87b43/ios/chrome/browser/signin/fake_signin_manager_builder.cc
[modify] https://crrev.com/df9b10be95d37143a1523091a9c66c9439c87b43/ios/chrome/browser/signin/signin_client_impl.mm
[modify] https://crrev.com/df9b10be95d37143a1523091a9c66c9439c87b43/ios/chrome/browser/signin/signin_manager_factory.cc
[modify] https://crrev.com/df9b10be95d37143a1523091a9c66c9439c87b43/ios/chrome/browser/sync/ios_chrome_profile_sync_service_factory.cc
[modify] https://crrev.com/df9b10be95d37143a1523091a9c66c9439c87b43/ios/chrome/browser/translate/chrome_ios_translate_client.mm
[modify] https://crrev.com/df9b10be95d37143a1523091a9c66c9439c87b43/ios/chrome/browser/web_state_list/web_state_opener_unittest.mm
[modify] https://crrev.com/df9b10be95d37143a1523091a9c66c9439c87b43/ios/web/public/test/web_view_interaction_test_util.mm
[modify] https://crrev.com/df9b10be95d37143a1523091a9c66c9439c87b43/ios/web/shell/shell_web_client.mm
[modify] https://crrev.com/df9b10be95d37143a1523091a9c66c9439c87b43/ios/web/web_state/web_state_impl.mm
[modify] https://crrev.com/df9b10be95d37143a1523091a9c66c9439c87b43/ios/web_view/internal/translate/web_view_translate_ranker_factory.cc

Owner: sdefresne@chromium.org
Status: Fixed (was: Available)
There are still a few references to this bug number in the code base. One more round of cleanup?
Status: Assigned (was: Fixed)
Ah, they are in src/chrome/ and I didn't look there when fixing this. Will take a look next week. Thank you for volunteering for reviewing the CL :-)

Comment 8 by vabr@chromium.org, Nov 13 2017

Cc: vabr@chromium.org
Sorry for the TODOs outside //ios, I guess I got confused about the scope when I put them there. If you want me to, I'll take care of removing or re-assigning under a different bug if still blocked on other platforms.
Labels: -OS-iOS
It is okay. I have https://chromium-review.googlesource.com/c/chromium/src/+/765658 to remove this. If it pass the CQ/approval/waterfall, then I'll close the issue. Otherwise, I'll just update it to correct OS where tool support is lagging and send it back to triage.
Project Member

Comment 10 by bugdroid1@chromium.org, Nov 13 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c444846ee27716d11200e10dc33e4e8f06ab697e

commit c444846ee27716d11200e10dc33e4e8f06ab697e
Author: Sylvain Defresne <sdefresne@chromium.org>
Date: Mon Nov 13 16:55:53 2017

Remove unnecessary std::move().

Bug:  703565 
Change-Id: Ic71b59bcff1c14e3e5a221951b58ba12893b1004
Reviewed-on: https://chromium-review.googlesource.com/765658
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515964}
[modify] https://crrev.com/c444846ee27716d11200e10dc33e4e8f06ab697e/chrome/browser/chromeos/extensions/info_private_api.cc
[modify] https://crrev.com/c444846ee27716d11200e10dc33e4e8f06ab697e/chrome/browser/extensions/api/proxy/proxy_api.cc

Status: Fixed (was: Assigned)
Cc: -vabr@chromium.org

Sign in to add a comment