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

[SofaCore] Merge TData in Data. #1753

Merged
merged 18 commits into from Feb 4, 2021

Conversation

damienmarchal
Copy link
Contributor

@damienmarchal damienmarchal commented Jan 28, 2021

I don't know what exactly was TData for, but in years no-one ever use it so I merged Data & TData to make things more clear.
I also re-implemented the existing feature using the child-delegation pattern instead of relying on call super.


By submitting this pull request, I acknowledge that
I have read, understand, and agree SOFA Developer Certificate of Origin (DCO).


Reviewers will merge this pull-request only if

  • it builds with SUCCESS for all platforms on the CI.
  • it does not generate new warnings.
  • it does not generate new unit test failures.
  • it does not generate new scene test failures.
  • it does not break API compatibility.
  • it is more than 1 week old (or has fast-merge label).

@damienmarchal damienmarchal added pr: status wip Development in the pull-request is still in progress refactoring Refactor code labels Jan 28, 2021
@damienmarchal damienmarchal changed the title [SofaCore] Remove TData to keep Data only. [SofaCore] Merge TData in Data. Jan 28, 2021
@damienmarchal damienmarchal added pr: status to review To notify reviewers to review this pull-request and removed pr: status wip Development in the pull-request is still in progress labels Feb 2, 2021
@epernod
Copy link
Contributor

epernod commented Feb 2, 2021

[ci-build][with-all-tests]

@damienmarchal
Copy link
Contributor Author

Thanks you very much @epernod for the review.

@hugtalbot
Copy link
Contributor

PR looks fine, but we absolutely need to define the convention about _doXXX_ or _isXXXX_
This should be definitely generalized within the whole code base AND the guidelines of SOFA.

@damienmarchal
Copy link
Contributor Author

So ready, isn it :)

@damienmarchal
Copy link
Contributor Author

damienmarchal commented Feb 3, 2021

So after discussion on gitter, the convention is
"each time you use the Template Method design pattern which delegate to its child private implementation then you must use the double _ to indicate it".
https://isocpp.org/wiki/faq/strange-inheritance#private-virtuals

If it is not private then feel free (which is exactly how is the sofa code base right now).

@damienmarchal
Copy link
Contributor Author

damienmarchal commented Feb 3, 2021

Appart from that, I did a quick search in the sofa code base, I didn't paid attention to how much of the doXXXX stuff are with a public visibility... which is clearly not what the design pattern suggested initially.

@jnbrunet jnbrunet merged commit a6679ef into sofa-framework:master Feb 4, 2021
@guparan guparan added this to the v21.06 milestone Feb 26, 2021
@hugtalbot hugtalbot added pr: status ready Approved a pull-request, ready to be squashed and removed pr: status to review To notify reviewers to review this pull-request labels May 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: status ready Approved a pull-request, ready to be squashed refactoring Refactor code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants