Skip to content
This repository was archived by the owner on Dec 20, 2023. It is now read-only.

Address Overloaded Virtual Warnings #70

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Address Overloaded Virtual Warnings #70

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2180d04
Address -Woverloaded-virtual warnings.
gerickson Oct 25, 2018
88da247
Address -Woverloaded-virtual warnings.
gerickson Oct 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/test-apps/MockDMPublisher.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/*
*
* Copyright (c) 2018 Google LLC.
* Copyright (c) 2016-2017 Nest Labs, Inc.
* All rights reserved.
*
Expand Down Expand Up @@ -34,7 +35,8 @@
* table.
*/

class MockDMPublisher : public nl::Weave::Profiles::DataManagement::DMPublisher
class MockDMPublisher __FINAL :
public nl::Weave::Profiles::DataManagement::DMPublisher
{
public:
/*
Expand All @@ -46,7 +48,9 @@ class MockDMPublisher : public nl::Weave::Profiles::DataManagement::DMPublisher

WEAVE_ERROR UpdateIndication(ExchangeContext *aResponseCtx, ReferencedTLVData &aDataList);

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

using DMPublisher::IncompleteIndication;
Copy link
Member

@robszewczyk robszewczyk Oct 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@gerickson gerickson Oct 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@gerickson gerickson Oct 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@jaylogue jaylogue Oct 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@gerickson gerickson Oct 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


#if WEAVE_CONFIG_WDM_ALLOW_PUBLISHER_SUBSCRIPTION

Expand Down
6 changes: 4 additions & 2 deletions src/test-apps/TestDataManagement.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ static WEAVE_ERROR ValidatePath(TLVReader &aReader,
* a sub-class of the WDM client and supply the relevant methods as follows.
*/

class DMTestClient :
class DMTestClient __FINAL :
public DMClient
{
public:
Expand Down Expand Up @@ -327,11 +327,13 @@ class DMTestClient :
return err;
}

void IncompleteIndication(const uint64_t &aPeerNodeId, StatusReport &aReport)
virtual void IncompleteIndication(const uint64_t &aPeerNodeId, StatusReport &aReport)
Copy link
Member

@robszewczyk robszewczyk Oct 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary virtual, but perhaps a beneficial override?

Copy link
Contributor Author

@gerickson gerickson Oct 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. See above.

Copy link
Contributor

@jaylogue jaylogue Oct 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@gerickson gerickson Oct 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #70 (comment).

{
printf("processing: <incomplete indication>\n");
}

using DMClient::IncompleteIndication;

#if WEAVE_CONFIG_WDM_ALLOW_CLIENT_SUBSCRIPTION

WEAVE_ERROR SubscribeConfirm(const uint64_t &aResponderId, StatusReport &aStatus, uint16_t aTxnId)
Expand Down
8 changes: 6 additions & 2 deletions src/test-apps/wdmtest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/*
*
* Copyright (c) 2018 Google LLC.
* Copyright (c) 2013-2017 Nest Labs, Inc.
* All rights reserved.
*
Expand Down Expand Up @@ -274,7 +275,7 @@ ReferencedTLVData DataList;
* a sub-class of the WDM client and supply the relevant methods as follows.
*/

class WDMTestClient :
class WDMTestClient __FINAL :
public DMClient
{
WEAVE_ERROR ViewConfirm(const uint64_t &aResponderId, StatusReport &aStatus, uint16_t aTxnId)
Expand Down Expand Up @@ -510,10 +511,13 @@ class WDMTestClient :
return err;
}

void IncompleteIndication(const uint64_t &aPeerNodeId, StatusReport &aReport)
virtual void IncompleteIndication(const uint64_t &aPeerNodeId, StatusReport &aReport)
Copy link
Member

@robszewczyk robszewczyk Oct 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary virtual but beneficial override?

Copy link
Contributor Author

@gerickson gerickson Oct 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. See above.

{
return;
}

using DMClient::IncompleteIndication;

};

int main(int argc, char *argv[])
Expand Down