BIND 10 #1773: build failure with clang++ on MacOS Lion

[BIND 10 Development]

#1773: build failure with clang++ on MacOS Lion
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:         |             Milestone:
  defect                             |  Sprint-20120417
                   Priority:         |            Resolution:
  medium                             |             Sensitive:  0
                  Component:  build  |           Sub-Project:  Core
  system                             |  Estimated Difficulty:  2
                   Keywords:         |           Total Hours:  0
            Defect Severity:  N/A    |
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:8 vorner]:
 > Hello
 >
 > First, should the `target_` be initilazed to `NULL` here?
 > {{{#!diff
 >      BenchMark(const int iterations, T target) :
 > -        iterations_(iterations), sub_iterations_(0), target_(target)
 > +        iterations_(iterations), sub_iterations_(0)
 >      {
 > -        initialize(true);
 > +        initialize(target, true);
 >      }
 > }}}

 Ah, yes, that's better (target_ shouldn't be used in the intended code
 path, but it's always a good practice to initialize everything).

 > Also, I'm not sure this is the correct fix. I guess this happened:
 >  * The parameter of the constructor is `T`, not `T&` here.
 >  * If left for autodetection, it may have happened gcc chose Type& as
 `T` and clang++ only Type. Anyway, the `T` got expanded to the bare type.
 >  * As such, the parameter is passed by copy allocated on the stack. The
 reference should be taken from the parameter on the stack which is
 obviously wrong, as the parameter ceases to exist soon.

 No, if the compiler consider T = Type& it wouldn't compile whether
 it's g++ or clang++ because in our usage `target` is a temporary
 object and this is a non const reference.  The following code
 shouldn't compile either by g++ or clang++:

 {{{#!c++
 struct Foo {
     explicit Foo(int&) {}
     //explicit Foo(int) {}
 };

 int
 main() {
     Foo(10);
     return (0);
 }
 }}}

 If we re-enabled the commented-out constructor, it should compile, and
 the second one should always be chosen because that's the only legal
 choice.

 As such,

 > I believe the correct fix in this case is not this workaround (because
 then the copied parameter would be initialized instead of the correct one
 anyway and it needs the type to be copy-constructible), but changing the
 constructor to `T& target` instead of `T target`.

 this cannot be a solution.   Using `const T& target` instead of
 `T target` would avoid this particular issue, but we cannot declare
 target as const because we need to be able to call non const member
 function of it (`run()`).

 I admit my workaround may not seem very clean, and if there's a
 workable alternative, I'm happy to use that.  But merely changing the
 argument type to a reference is not a workable solution.

 > Also, did you notice the commit message has two [branch] tags?

 Ah, I didn't, but I now see it and I know why.  It should be
 automatically done when I cherry-picked it from the older branch.
 That should be a one time garbage.

-- 
Ticket URL: <http://bind10.isc.org/ticket/1773#comment:9>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development