MWC: make Dummy function placeholders callable to fix build with MSVC

[678098]

Simpler to observe what's going on in this source: https://github.com/bloomberg/blazingmq/blob/49fe8ab987bdaeef1298bb16538b373922b8d73f/src/groups/mwc/mwcex/mwcex_job.h

Note that in the code we instantiate some template classes with Dummy objects, like this:

mwcu::ObjectPlaceHolder<sizeof(Job_Target<Dummy>)> d_target;

We don't actually use these Job_Target<Dummy> objects, but let's look closely what do we want to instantiate:

bslmf::Util::moveIfSupported(d_function.object())();

Here:

blazingmq/src/groups/mwc/mwcex/mwcex_job.h

Lines 217 to 222 in 49fe8ab

	// MANIPULATORS
	template <class FUNCTION>
	inline void Job_Target<FUNCTION>::invoke()
	{
	bslmf::Util::moveIfSupported(d_function.object())();
	}

So basically in this code we are trying to "call" a Dummy structure here: bslmf::Util::moveIfSupported(d_function.object())();

I am curious why it even compiled before.

Note that we already have this fix in some, but not all the places where it's needed. For example, here:

blazingmq/src/groups/mwc/mwcu/mwcu_operationchain.h

Lines 390 to 392 in 7f817c4

	struct Dummy : public mwcu::NoOp {
	void* d_padding[5];
	};

UPD: for executor it's even more complicated, since we have to implement the common interface of Executor_TargetBase which is expected as a template parameter.

[hallfox]

Why was MSVC fixed by making these types callable? I'm a little concerned we're taking a PoD type and unmaking it PoD.

[678098]

@hallfox added an explanation for this to the title of the PR

[ddragan-bloomberg]

I'll review this.

[ddragan-bloomberg]

Template code is not instantiated unless used explicitly, which I believe is the reason this code compiles perfectly in GCC/Clang. MSVC might by buggy. Does libmwc even have to compile on Windows? If so, what is the minimum compiler version is has to compile with?

[ddragan-bloomberg]

I kinda get the rational behind changes in mwcex_job -- the Dummy is involved with a functor, but what is the rational here? Does the code not compile if you leave this dummy (and the one below) as is?

[ddragan-bloomberg]

Side note. Evgenii, while you're at it, might as well replace usage of mwcu::NoOp with bdlf::NoOp

[ddragan-bloomberg]

wouldn't it be simpler to remove those backup files in that same script that calls sim_cpp11_features.pl? That way we won't have to edit this file each time a component is added or removed.

[678098]

We don't have access to the script that launches the sim_cpp11_features.pl, since it's done within bde-tools. I updated .gitignore with wildcard backup pattern which covers all the changes, and this is the simplest way.

[ddragan-bloomberg]

Evgenii, since you decided to replace mwcu::NoOp in all mwc and BMQ code, please do this in a separate PR, then rebase this PR on top of the other one once I merge it.

And DO NOT remove the mwcu_noop component! Other libraries and apps depend on libmwc and might be still using mwcu::NoOp.

[678098]

And DO NOT remove the mwcu_noop component! Other libraries and apps depend on libmwc and might be still using mwcu::NoOp.

I already checked that no other libraries depend on mwcu::NoOp

[ddragan-bloomberg]

I already checked that no other libraries depend on mwcu::NoOp

Okay, It that case we are good to remove it.

[ddragan-bloomberg]

Also note than, unlike mwc, BDE provides s static constexpr instance of bdlf::NoOp -- bdlf::noOp, should use it where appropriate.

[678098]

Also note than, unlike mwc, BDE provides s static constexpr instance of bdlf::NoOp -- bdlf::noOp, should use it where appropriate.

This is a good point. We might save a few ticks in some places.

Evgenii, since you decided to replace mwcu::NoOp in all mwc and BMQ code, please do this in a separate PR, then rebase this PR on top of the other one once I merge it.

Moved out mwcu::NoOp -> bdlf::NoOp transition from this branch.

Does libmwc even have to compile on Windows?

We can get Windows support for mwc almost for free, a bit more complicate for lib bmq and even more complicated for mqb. So, we can pretty easily make buildable Windows SDK (depends on mwc + bmq), and it's more difficult to build a Windows Broker (depends on mwc + bmq + mqb).

For mwc, just fix these few templates and that's it. We don't have requirement to build BlazingMQ on Windows, but we can at least gradually extend support for this platform, by fixing a few non-portable parts of the codebase. There are external users who might benefit from using it on Windows #314

If so, what is the minimum compiler version is has to compile with?

No strict requirements. On this stage, it's worth it to solve problems once they occur, until we have a better vision.

[ddragan-bloomberg]

How do you check the code compiles on Windows. Which compiler do you use? Can you use a newer compiler?

[678098]

How do you check the code compiles on Windows. Which compiler do you use? Can you use a newer compiler?

I have a configured Windows preset which I can build. Also, I use the latest MSVC compiler available:

$ "Tools\MSVC\14.40.33807\bin\HostX64\x64\CL.exe"
Microsoft (R) C/C++ Optimizing Compiler Version 19.40.33812 for x64

If I roll back the proposed changes, I will get errors from the compiler:

D:\blazingmq\src\groups\mwc\mwcex\mwcex_job.h(224,33): error C2064: term does not evaluate to a function taking 0 arguments [D:\blazingmq\cmake.bld\src\groups\mwc\mwcex-iface.vcxproj]
  mwcex_promise.cpp
  mwcex_promise_cpp03.cpp
  mwcex_sequentialcontext.cpp
D:\blazingmq\src\groups\mwc\mwcex\mwcex_job.h(224,33): error C2064: term does not evaluate to a function taking 0 arguments [D:\blazingmq\cmake.bld\src\groups\mwc\mwcex-iface.vcxproj]
  mwcex_strand.cpp
D:\blazingmq\src\groups\mwc\mwcex\mwcex_job.h(224,33): error C2064: term does not evaluate to a function taking 0 arguments [D:\blazingmq\cmake.bld\src\groups\mwc\mwcex-iface.vcxproj]
  mwcex_systemexecutor.cpp
  Generating Code...

[ddragan-bloomberg]

There is a question left unanswered -- what is the rational behind changes in mwcex_future.h? Does the code compiles without them?

[678098]

There is a question left unanswered -- what is the rational behind changes in mwcex_future.h? Does the code compiles without them?

It doesn't

[ddragan-bloomberg]

I assume this is because Future_Callback is functor-related. What about Future_Exception? Are you able to remove the use of NoOp there?

[678098]

I assume this is because Future_Callback is functor-related. What about Future_Exception? Are you able to remove the use of NoOp there?

No

If I remove inheritance from NoOp in either Future_Callback or Future_Exception, it will cause the same compilation error in Job_Target::invoke():

// MANIPULATORS
template <class FUNCTION>
inline void Job_Target<FUNCTION>::invoke()
{
    bslmf::Util::moveIfSupported(d_function.object())(); // blazingmq\src\groups\mwc\mwcex\mwcex_job.h(224,33): error C2064: term does not evaluate to a function taking 0 arguments [blazingmq\cmake.bld\src\groups\mwc\mwcex-iface.vcxproj]

}

And regarding the Executor_Box_SboImp::Dummy, if left as-is, the compilation errors would be:

blazingmq\src\groups\mwc\mwcex\mwcex_executor.h(603,52): error C2676: binary '==': 'const EXECUTOR' does not define this operator or a conversion to a type acceptable to the predefined operator [blazingmq\cmake.bld\src\groups\mwc\mwcex-iface.vcxproj]
blazingmq\src\groups\mwc\mwcex\mwcex_executor.h(603,52): error C2676:         with [blazingmq\cmake.bld\src\groups\mwc\mwcex-iface.vcxproj]
blazingmq\src\groups\mwc\mwcex\mwcex_executor.h(603,52): error C2676:         [ [blazingmq\cmake.bld\src\groups\mwc\mwcex-iface.vcxproj]
blazingmq\src\groups\mwc\mwcex\mwcex_executor.h(603,52): error C2676:             EXECUTOR=BloombergLP::mwcex::Executor_Box_SboImp::Dummy [blazingmq\cmake.bld\src\groups\mwc\mwcex-iface.vcxproj]
blazingmq\src\groups\mwc\mwcex\mwcex_executor.h(603,52): error C2676:         ] [blazingmq\cmake.bld\src\groups\mwc\mwcex-iface.vcxproj]

I found out that it's not necessary to inherit Executor_Box_SboImp::Dummy from Executor_TargetBase, but I had to provide implementations of two methods, absence of which causes compilation errors:

blazingmq\src\groups\mwc\mwcex\mwcex_executor.h(639,52): error C2676: binary '==': 'const EXECUTOR' does not define this operator or a conversion to a type acceptable to the predefined operator [blazingmq\cmake.bld\src\groups\mwc\mwcex-iface.vcxproj]

blazingmq\src\groups\mwc\mwcex\mwcex_executortraits.h(206,14): error C2039: 'post': is not a member of 'BloombergLP::mwcex::Executor_Box_SboImp::Dummy' [blazingmq\cmake.bld\src\groups\mwc\mwcex-iface.vcxproj]

[ddragan-bloomberg]

Looks good