Skip to content
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.

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

questions on interfaces behaviour #29

Closed
yerden opened this issue Mar 22, 2018 · 5 comments
Closed

questions on interfaces behaviour #29

yerden opened this issue Mar 22, 2018 · 5 comments

Comments

@yerden
Copy link
Contributor

yerden commented Mar 22, 2018

Must the method implementation check for m_owned and do SafeDelete()?

virtual bool remove(const T value)=0;

For example, here, you say:

// in this case, it is no point to check m_owned and SafeDelete value

What's the expected behaviour here?

@dingmaotu
Copy link
Owner

The exact behavior of this method should be checking for owning status and delete the element if necessary. If T is not a pointer, it is not necessary. But for the Vector case, what I was thinking is that if I can pass a pointer to the container, then I at least have a reference of the object and if I want to remove it from the container then I may not want to invalidate my reference at the same time. So the behavior here is not standard but it is reasonable. I don't know how to reconcile this obvious conflict. Do you have any idea?

@yerden
Copy link
Contributor Author

yerden commented Mar 26, 2018

If Collection class has public m_owned flag then we have to support it in every inherited implementation. Thus, if m_owned is set, Vector implementation MUST destroy the element which is thrown out of it. Otherwise, there's no point in having m_owned flag in the first place. So must do every other derived class. So I don't think I get your point here 😉

Globally, I think that the library must have consistent, non-contradictory API throughout all the code base. Ideally, all these caveats must be documented, or at least (which is more important) all derived classes must display the same behaviour hinted by their ancestors.

@dingmaotu
Copy link
Owner

OK, I think you are right. I will fix the inconsistency ASAP.

@yerden
Copy link
Contributor Author

yerden commented Apr 9, 2018

Hi, any success? I'd be glad if you process the PR backlog out there 😄

@dingmaotu
Copy link
Owner

Not yet. Sorry for the slow response to your PR. I am extremely busy since I changed my job... I will try to process your PRs tonight. Thanks for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants