Remove old C++03 move emulation code.

chrome/browser/printing/pdf_to_emf_converter.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/printing/pdf_to_emf_converter.h"
 
 #include <queue>
 
 #include "base/files/file.h"
 #include "base/files/file_util.h"
@@ skipping 95 matching lines @@
 
   void Stop();
 
   // UtilityProcessHostClient implementation.
   void OnProcessCrashed(int exit_code) override;
   void OnProcessLaunchFailed() override;
   bool OnMessageReceived(const IPC::Message& message) override;
 
  private:
   class GetPageCallbackData {
-    MOVE_ONLY_TYPE_FOR_CPP_03(GetPageCallbackData, RValue);
+    MOVE_ONLY_TYPE_FOR_CPP_03(GetPageCallbackData);
 
    public:
     GetPageCallbackData(int page_number,
                         PdfToEmfConverter::GetPageCallback callback)
         : page_number_(page_number), callback_(callback) {}
 
     // Move constructor for STL.

[danakj, 2015/10/15 23:35:06]

uh maybe kill these comments? they're odd

[dcheng, 2015/10/16 00:40:01]

Ah, I meant to comment on this.

So it's really weird that this comment would say "move constructor for STL",
because there's no way STL containers would explicitly support our custom RValue
emulation. I think this happens to work because std::queue never reallocates
internal buffers the way std::vector would, so it never needed to actually move
elements internally. And std::queue::push(GetPageCallbackData(...)) just
happened to tickle the RValue conversion path, allowing a move to take place.

Of course, the problem is we're now using C++11 library features and we're not
supposed to... so I'm not quite sure how to resolve this. +thakis

[danakj, 2015/10/16 00:44:24]

Uh, well, the old RValue things would not have been used by STL, cuz you have to
Pass() to actually move. If RVO happens to kick in more that seems okay?

[dcheng, 2015/10/16 01:30:29]

Thinking about this more, my original line of reasoning is probably completely
wrong.

... that being said, I think that this block of code must already depend on
C++11 library features.

In C++03, push has the signature std::queue<T>::push(const T&). One condition
for copy elision is:
  "When a nameless temporary, not bound to any references, would be moved or
copied into an object of the same type (ignoring top-level cv-qualification),
the copy/move is omitted" [1]

But passing a T to a const T& would not be a copy or a move of a nameless
temporary. Following the chain of stuff, C++03 eventually uses placement new to
copy construct the (no longer nameless) argument:

      // From libstdc++:
      void 
      construct(pointer __p, const _Tp& __val) 
      { ::new((void *)__p) _Tp(__val); }

Copy elision would also not apply here. Since __val is a const lvalue reference:

  1. it can't bind to the copy constructor (which takes a mutable reference)
  2. we can't use any user-defined conversions, because they are all non-const.

Thus, this shouldn't even compile in C++03.

However, the C++11 version uses perfect forwarding:
      template<typename _Up, typename... _Args>
        void
        construct(_Up* __p, _Args&&... __args)
	{ ::new((void *)__p) _Up(std::forward<_Args>(__args)...); }

and in C++11, there's an additional overload: std::queue<T>::push(T&&). Since we
have a temporary, it binds to the T&& overload. Then, inside construct(...),
Args&& expands to GetPageCallbackData&& &&, which collapses to
GetPageCallbackData&&. At this point, I think the rule about using one
user-defined conversion kicks in, operator RValue() gets invoked (since |args|
is not const), and GetPageCallbackData is constructed in place using the RValue
constructor.

[1] http://en.cppreference.com/w/cpp/language/copy_elision

[dcheng, 2015/10/19 21:29:50]

Done.

[danakj, 2015/10/19 21:49:24]

Wow ya, I agree. The problem is they are putting a move-only type into an STL
container. And whether they do that with our emulation via T::Rvalue or not,
it's possible to do as long as the container doesn't try to move it again later?

We should remove the move-only bits here, but this class is holding a (callback
containing a) scoped_ptr. So a more significant refactor would actually be
needed.

This code is only built on windows which is why it is working:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/BUI...

Maybe just adding a scary TODO comment about at the call to push that it is
pushing a move-only type which is not possible on all platforms and it is not
allowed.. and we can remove it one day?

-    GetPageCallbackData(RValue other) { this->operator=(other); }
+    GetPageCallbackData(GetPageCallbackData&& other) { *this = other.Pass(); }
 
     // Move assignment for STL.
-    GetPageCallbackData& operator=(RValue rhs) {
-      page_number_ = rhs.object->page_number_;
-      callback_ = rhs.object->callback_;
-      emf_ = rhs.object->emf_.Pass();
+    GetPageCallbackData& operator=(GetPageCallbackData&& rhs) {
+      page_number_ = rhs.page_number_;
+      callback_ = rhs.callback_;
+      emf_ = rhs.emf_.Pass();
       return *this;
     }
 
     int page_number() const { return page_number_; }
     const PdfToEmfConverter::GetPageCallback& callback() const {
       return callback_;
     }
     ScopedTempFile TakeEmf() { return emf_.Pass(); }
     void set_emf(ScopedTempFile emf) { emf_ = emf.Pass(); }
 
@@ skipping 360 matching lines @@
 
 PdfToEmfConverter::~PdfToEmfConverter() {
 }
 
 // static
 scoped_ptr<PdfToEmfConverter> PdfToEmfConverter::CreateDefault() {
   return scoped_ptr<PdfToEmfConverter>(new PdfToEmfConverterImpl());
 }
 
 }  // namespace printing