[kudu-CR] [util] conventional signature for Status::operator=()

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [util] conventional signature for Status::operator=()
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6175/2/src/kudu/util/status.h
File src/kudu/util/status.h:

PS2, Line 148: throw();
> I think it prevents something like:
Right, it's needed for 'chaining' assignments like Todd mentioned: it's 
necessary to return reference to the left-hand operand to make it possible 
(returning reference to the right-hand operand is also an option but might 
complicate things in some scenarios).

And yes, as Adar mentioned, it seems to be ABI compatible.  Just in case, I 
played with a small test to verify that:

class Status {
  public:
    Status(int n = 0);
    //void operator=(const Status& s);
    Status& operator=(const Status& s);

    int n() const;

  private:
    int n_;
};

In both versions for assignment operation the dynamic library compiled had the 
same set of symbols:

aserbin-MBP:op-test$ nm -a libstatus.so.*

libstatus.so.new_signature:
0000000000000f50 T __ZN6StatusC1Ei
0000000000000f30 T __ZN6StatusC2Ei
0000000000000f80 T __ZN6StatusaSERKS_
0000000000000fa0 T __ZNK6Status1nEv
                 U dyld_stub_binder

libstatus.so.old_signature:
0000000000000f50 T __ZN6StatusC1Ei
0000000000000f30 T __ZN6StatusC2Ei
0000000000000f80 T __ZN6StatusaSERKS_
0000000000000fa0 T __ZNK6Status1nEv
                 U dyld_stub_binder

The assignment operator does not change its symbol when changing the return 
type:

aserbin-MBP:op-test$ c++filt __ZN6StatusaSERKS_
Status::operator=(Status const&)

It explains why it's not possible to have multiple overloaded function 
signatures different only in return type :)

As for the API compatibility, the new signature of the assignment operator 
broadens the list of expressions where it can be used, so I wouldn't expect 
anything to break from the API perspective.


-- 
To view, visit http://gerrit.cloudera.org:8080/6175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If04674c88d97204d52bcc15a40755d556f309ea1
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <danburk...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes