Skip to content

Add user context to library #206

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Add user context to library #206

wants to merge 3 commits into from

Conversation

alonbl
Copy link
Contributor

@alonbl alonbl commented Oct 9, 2023

The current overwrite method of weak pointer is a global overwrite. As class is not polymorphic type (aka the overwrite enabled methods are weak instead of virtual) and there is no vtable as none of the methods are virtual, there is no way to have instance specific overwrite.

A user context will provide a method to call back into a context of a parent class and resolve the issue without making the class polymorphic.

For example:

class MY_SFE_UBLOX_GNSS: public SFE_UBLOX_GNSS {
public:
    MY_SFE_UBLOX_GNSS() {
        setUserContext(this);
    }
    void myProcessRTCM(uint8_t incoming) {}
};

void SFE_UBLOX_GNSS::processRTCM(uint8_t incoming) {
    static_cast<
        MY_SFE_UBLOX_GNSS *
    >(getUserContext())->myProcessRTCM(incoming);
}

@alonbl
Copy link
Contributor Author

alonbl commented Oct 9, 2023

Another option is to add virtual destructor to the SFE_UBLOX_GNSS class to allow this to be dynamic casted to the child class.

@PaulZC
Copy link
Collaborator

PaulZC commented Oct 9, 2023

Hi Alon (@alonbl ),

This is a really nice Pull Request - thank you. We started using ((weak)) functions in the original version of this library - and then kept using them... If I'm reading it right, this bridges the gap nicely, maintaining backward compatibility but allowing a better derived-class implementation too.

I'm going to point this at the release_candidate branch and run the workflow on it, just to make sure there are no hidden surprises.

Do you have time to update Example2, showing how that can take advantage of the new methods?

Thanks again,
Paul

@PaulZC PaulZC changed the base branch from main to release_candidate October 9, 2023 16:25
@alonbl
Copy link
Contributor Author

alonbl commented Oct 9, 2023

Hi @PaulZC ,

Thank you for the fast response :)

Sure I will update the example.

Will you prefer this method or adding virtual destractor? I mean, is there a reason why we do not allow inheritance?

Alon

@PaulZC
Copy link
Collaborator

PaulZC commented Oct 9, 2023

Hi Alon,

I think I prefer the user context over the virtual destructor... It seems clear and straightforward to me. I don't think other users will have any problem understanding it or using it.

Best wishes,
Paul

@alonbl
Copy link
Contributor Author

alonbl commented Oct 9, 2023

OK, just in case, please review[1] which is the virtual alternative.
I will update the example to meet this patch as well, so we can compare the two alternatives.

[1] #207

@alonbl
Add user context to library
5f50370 
The current overwrite method of weak pointer is a global overwrite. As class
is not polymorphic type (aka the overwrite enabled methods are weak instead of
virtual) and there is no vtable as none of the methods are virtual, there is
no way to have instance specific overwrite.

A user context will provide a method to call back into a context of a parent
class and resolve the issue without making the class polymorphic.

Signed-off-by: Alon Bar-Lev <alon.barlev@gmail.com>
@alonbl
Copy link
Contributor Author

alonbl commented Oct 9, 2023

Hi @PaulZC,

Please compare between #206 and #207 , I believe #207 is much cleaner.
In any case we can support both methods... :)

Please let me know what you think.

Alon

@PaulZC
Copy link
Collaborator

PaulZC commented Oct 9, 2023

Hi Alon,

Agreed! #207 is cleaner.

Closing this in favour of #207.

Thanks again,
Paul

@PaulZC PaulZC closed this Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alonbl @PaulZC