eshiehz / crashpad

Automatically exported from code.google.com/p/crashpad
0 stars 0 forks source link

ScopedGeneric::Pass may not work correctly #14

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
What steps will reproduce the problem?
ScopedGeneric cannot be assigned to from the result of .Pass().

Using the specialization of ScopedFileHandle, the following DOES work:

  ScopedFileHandle Open() {
    ScopedFileHandle handle(open(…));
    return handle.Pass();
  }

  void Frobinate() {
    ScopedFileHandle handle(Open());
  }

But when written like such, Frobinate does NOT compile:

  void Frobinate(bool goaty) {
    ScopedFileHandle handle;
    if (goaty)
      handle = Open();
  }

One must do this instead:

  void Frobinate(bool goaty) {
    ScopedFileHandle handle;
    if (goaty)
      handle.reset(Open().release());
  }

If the frobination of goats is too abstract, this is an issue here: 
https://chromium.googlesource.com/crashpad/crashpad//+/1a635e3a799c9c93b39d1a9f5
07b3c9eebbd873b/client/settings.cc#218.

When switching from a.reset(o.release()) to |a = o|, the following error is 
encountered:

rsesek@rsesek-retinapro:/Users/rsesek/Projects/crashpad(settings) % ninja -C 
out/Release crashpad_client            
ninja: Entering directory `out/Release'
[1/2] CXX obj/client/crashpad_client.settings.o
FAILED: c++ -MMD -MF obj/client/crashpad_client.settings.o.d -DNDEBUG -I../.. 
-I../../compat/mac -I../../compat/non_win 
-I../../third_party/mini_chromium/mini_chromium -O3 -gdwarf-2 
-fvisibility=hidden -Werror -Wnewline-eof -arch x86_64 -Wall -Wendif-labels 
-Wextra -Wno-unused-parameter -Wno-missing-field-initializers 
-Wexit-time-destructors -Wheader-hygiene -Wno-selector-type-mismatch 
-Wsign-compare -Wstring-conversion -std=c++11 -fno-rtti -fno-exceptions 
-fvisibility-inlines-hidden -fno-threadsafe-statics -fno-strict-aliasing 
-fstack-protector-all  -c ../../client/settings.cc -o 
obj/client/crashpad_client.settings.o
../../client/settings.cc:218:19: error: no viable overloaded '='
    scoped_handle = OpenForReadingAndWriting();
    ~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~
../../third_party/mini_chromium/mini_chromium/base/scoped_generic.h:19:3: note: 
candidate function not viable: expects an l-value for 1st argument
  MOVE_ONLY_TYPE_FOR_CPP_03(ScopedGeneric, RValue)
  ^
../../third_party/mini_chromium/mini_chromium/base/move.h:213:8: note: expanded 
from macro 'MOVE_ONLY_TYPE_FOR_CPP_03'
  void operator=(type&); \
       ^
1 error generated.
ninja: build stopped: subcommand failed.

What is the expected output? What do you see instead?
This is contrary to the ScopedGeneric documentation. I should be able to assign 
the rvalue of Pass() to a ScopedGeneric lvalue.

Please use labels and text to provide additional information.

Original issue reported on code.google.com by rsesek@chromium.org on 9 Mar 2015 at 11:02

GoogleCodeExporter commented 9 years ago
ScopedGeneric provides ScopedGeneric(RValue) but not operator=(RValue). This is 
the same as upstream. I don’t see where it says in the ScopedGeneric 
documentation that this is supposed to work.

I think that this is a reasonable request, but it would need to appear upstream 
in order to bring it into mini_chromium, so for now it’s WontFix in this 
project’s bug database.

Original comment by mark@chromium.org on 10 Mar 2015 at 6:15

GoogleCodeExporter commented 9 years ago
This is the documentation that led me to believe this should work: 
https://code.google.com/p/chromium/codesearch#chromium/src/base/move.h&q=move.h&
sq=package:chromium&type=cs&l=111

Original comment by rsesek@chromium.org on 10 Mar 2015 at 6:17

GoogleCodeExporter commented 9 years ago
That example does show an operator=(RValue) being defined. Earlier in the 
move.h documentation, it says that the class needs to define its own: 
https://code.google.com/p/chromium/codesearch#chromium/src/base/move.h&q=move.h&
sq=package:chromium&type=cs&l=26

Original comment by mark@chromium.org on 10 Mar 2015 at 6:19

GoogleCodeExporter commented 9 years ago
Upstream ScopedGeneric does: 
https://code.google.com/p/chromium/codesearch#chromium/src/base/scoped_generic.h
&sq=package:chromium&type=cs&l=87

Original comment by rsesek@chromium.org on 10 Mar 2015 at 6:21

GoogleCodeExporter commented 9 years ago
That’s a move constructor, not operator=. I thought we were talking about 
operator=.

Original comment by mark@chromium.org on 10 Mar 2015 at 6:23

GoogleCodeExporter commented 9 years ago
Oh, I guess that' only the ctor and not operator=. Nevermind.

Original comment by rsesek@chromium.org on 10 Mar 2015 at 6:23

GoogleCodeExporter commented 9 years ago
mini_chromium’s move.h and scoped_generic.h are identical to the 
upstream’s, except for some insignificant comment differences.

Original comment by mark@chromium.org on 10 Mar 2015 at 6:24

GoogleCodeExporter commented 9 years ago
https://code.google.com/p/chromium/issues/detail?id=453142

Original comment by rsesek@chromium.org on 10 Mar 2015 at 6:35

GoogleCodeExporter commented 9 years ago
https://codereview.chromium.org/997953002/ : 
https://chromium.googlesource.com/chromium/mini_chromium/+/b3d221e85747409f1ffb0
81ff958a1df7eb6be29

Original comment by rsesek@chromium.org on 11 Mar 2015 at 6:02