Address Overloaded Virtual Warnings

[gerickson]

No description provided.

[codecov-io]

Codecov Report

Merging #70 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master      #70      +/-   ##
==========================================
+ Coverage   60.13%   60.13%   +<.01%     
==========================================
  Files         306      306              
  Lines       49366    49367       +1     
==========================================
+ Hits        29685    29686       +1     
  Misses      19681    19681

Impacted Files	Coverage Δ
src/test-apps/TestSystemObject.cpp	97.56% <0%> (+0.01%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9726ae3...88da247. Read the comment docs.

[robszewczyk]

This code seems to be most relevant to the Legacy WDM code. If clang is complaining, it is perhaps complaining for the wrong reasons; the IncompleteIndication is supposed to override / implement the pure virtual from ProtocolEngine.

[robszewczyk]

I don't quite understand. I believe the the intention here was to have the method above override the DMPublisher::IncompleteIndication; Arguably, it would be appropriate to add the __OVERRIDE macro to the IncompleteIndication. Because the IncompleteIndication already is declared virtual in base, there is no need to declare it virtual here, right?

[gerickson]

I agree, this seems an odd warning. As Jay and I discussed this morning, it might make sense long-term to get rid of "-Wall" in configure.ac and select those warnings we feel actually make sense and add real, measurable value.

Using __OVERRIDE here might be better though that may then trigger -Winconsistent-missing-override. I'll give it a try.

[gerickson]

The using statement here seems to be the required magic but it doesn't make much sense to me based on my knowledge of the language.

The better approach here might be just to disable -Woverloaded-virtual (-Wno-overloaded-virtual) for clang. Thoughts @robszewczyk or @jaylogue?

[jaylogue]

IncompleteIndication() has a different signature on the base class:
void IncompleteIndication(Binding *aBinding, StatusReport &aReport)
vs.
void IncompleteIndication(const uint64_t &aPeerNodeId, StatusReport &aReport)
This is likely erroneous, as nothing appears to call the subclass method.

[gerickson]

Looking at this again, src/lib/profiles/data-management/Legacy/ProtocolEngine.h defines both:

virtual void IncompleteIndication(Binding *aBinding, StatusReport &aReport);

and:

virtual void IncompleteIndication(const uint64_t &aPeerNodeId, StatusReport &aReport) = 0;

The implementation of the former calls the latter making the base class abstract:

void ProtocolEngine::IncompleteIndication(Binding *aBinding, StatusReport &aReport)
{
    bool indicated = FailTransactions(aBinding, aReport);

    if (!indicated)
        IncompleteIndication(aBinding->mPeerNodeId, aReport);
}

It seems like the test clients correctly override the latter which seems to imply that the warning is a bit pedantic.

[robszewczyk]

Unnecessary virtual, but perhaps a beneficial override?

[gerickson]

Agreed. See above.

[jaylogue]

As with MockDMPublisher I think this is an error, as nothing calls DMTestClient::IncompleteIndication(const uint64_t &aPeerNodeId, StatusReport &aReport).

[gerickson]

See #70 (comment).

[robszewczyk]

Unnecessary virtual but beneficial override?

[gerickson]

Agreed. See above.