NFD » ndn-cxx

This is not enough. There needs to be a way to let the library consumer know that monitoring is supported/not supported on the current platform, at either compile- or run-time.

Junxiao Shi wrote:

It is intentionally designed as an instance method rather than a static method, because #4024 would need to accept Impl* as a constructor parameter in order to accommodate mock impl, after which having a static NetworkMonitor::getCapabilities() function would be infeasible.

Can you elaborate on this requirement? I'm not sure I'm following.

As indicated in #4024-4, NetworkMonitor constructor takes unique_ptr<ImplBase>. Therefore, the capabilities of a NetworkMonitor can differ per instance. This allows getCapabilities() return value to be mocked.

The problem with requiring an instance on which to call getCapabilities() is that, once the constructor of NetworkMonitor is called, things already start happening (sockets are created, async work is scheduled, etc...), so it might already be too late.

Reply to note-8:

How is that "too late"? The caller is supposed to connect all signals as soon as NetworkMonitor is constructed. getCapabilities() is purely informational.

Junxiao Shi wrote:

How is that "too late"? The caller is supposed to connect all signals as soon as NetworkMonitor is constructed. getCapabilities() is purely informational.

With "too late" I mean that a lot of work has already happened in the constructor of NetworkMonitor, so it's wasteful to construct an instance just to query its capabilities, which don't actually depend on the instance itself but on the platform on which NetworkMonitor is running.

The idiomatic way, as Alex also said in code review, is using a static member function. Do we really need to mock getCapabilities() itself?

it's wasteful to construct an instance just to query its capabilities

As commented in https://gerrit.named-data.net/#/c/3811/1/src/util/face-uri.cpp@478, each application is expected to have only one instance of NetworkMonitor, so there would be no waste.

Do we really need to mock getCapabilities() itself?

Yes, it's necessary to mock everything including getCapabilities. For example, the calling app may launch a polling session for MTU changes, if MTU signals are not supported but needed by the app. To test this logic, getCapabilities must be mocked.

Junxiao Shi wrote:

it's wasteful to construct an instance just to query its capabilities

As commented in https://gerrit.named-data.net/#/c/3811/1/src/util/face-uri.cpp@478, each application is expected to have only one instance of NetworkMonitor, so there would be no waste.

The waste would happen when an app realizes that it cannot use the NetworkMonitor it just constructed because it doesn't provide the required functionality, and therefore destructs it because the app has an internal fallback or can work without it (e.g. it's an optional feature). Anyway, this might be a very minor concern that we can ignore if there's no other solution.