Fix multiple copy constructors warning

[hughbe]

The problem is that we define a T& as well as a const T& copy constructor, which MSVC doesn't like.

@jckarter could you let me know if this is a valid fix or if we should just keep silencing this warning with a pragma.

[jckarter]

Should this be a T&& constructor instead of a T& constructor? Overloading on const/non-const lvalue ref feels brittle to me. I think @eeckstein or @swiftix wrote this code originally.

[hughbe]

I've tried that - MSVC compiles everything fine, let's see if Clang does too

[hughbe]

@swiftix @eeckstein could you take a quick look and reveal the intent of having two copy constructors - if you have any suggestions I'd be happy to listen

[swiftix]

@hughbe Hi, I think the original code was written by me. I had a very quick look to see why we have two constructors. If I just remove the const constructor, I see the following:

swift/lib/SILOptimizer/Utils/CheckedCastBrJumpThreading.cpp:250:10: note: in instantiation of function template specialization 'llvm::Optional<swift::BasicBlockCloner>::emplace<swift::BasicBlockCloner>' requested here
  Cloner.emplace(BasicBlockCloner(CCBBlock));
         ^
swift/include/swift/SILOptimizer/Utils/Local.h:394:7: note: candidate constructor (the implicit copy constructor) not viable: expects an l-value for 1st argument
class BasicBlockCloner : public BaseThreadingCloner {
      ^
swift/include/swift/SILOptimizer/Utils/Local.h:396:5: note: candidate constructor not viable: no known conversion from 'swift::BasicBlockCloner' to 'swift::SILBasicBlock *' for 1st argument
    BasicBlockCloner(SILBasicBlock *From, SILBasicBlock *To = nullptr,
    ^
1 error generated.

And this seems to be the only place where the const copy constructor is used. It is used implicitly here, because it is probably a part of the cloner or SILBuilder.

So, my impression is that it was introduced only to handle this case. If you can re-factor the code to make it work with just one copy constructor, I think we should be fine.

Please let me review your re-factored version before merging.

[hughbe]

Take a look at the push, it incurs new/delete operations but It compilers, fixes the warning and tests successfully

[swiftix]

It looks cleaner, but I don't like the use of new/delete. Could you try to move the creation of the cloner instance into CheckedCastBrJumpThreading::optimizeFunction, which calls both functions that set the Cloner value? You could allocate the Cloner there on stack and just pass its address or something like that. Or may be the Cloner instance can be created without the new/delete as part of the Edit constructor and becomes a part of the Edit structure, because that one sets the CCBlock which actually may need a cloner later?

[hughbe]

@swiftix I've updated this PR - should be a simple review, and it removes all the strange code I added in my previous attempts!

[swiftix]

@hughbe LGTM. Nice catch about emplace! I really liked it :-)

[jrose-apple]

The two constructors do very different things today. I'm suspicious of just changing this signature without actually figuring out what the intent is.

[hughbe]

@swift-ci please smoke test

[hughbe]

Looks like I've found the correct fix - llvm::Optional<T>::emplace takes the actual arguments to construct the object - this means that we were trying to do BasicBlockCloner(BasicBlockCloner(CCBlock)) instead of BasicBlockCloner(CCBlock)