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

Fix PropertyVector<T*> for multi-component case #1441

Merged
merged 7 commits into from
Sep 27, 2016

Conversation

norihiro-w
Copy link
Collaborator

Currently PropertyVector<T*> works only with a single component.

@@ -185,6 +197,20 @@ friend class Properties;
return t;
}

//! Returns the value for the given component stored in the given tuple.
T const& getComponent(std::size_t tuple_index,
std::size_t component) const
Copy link
Member

@TomFischer TomFischer Sep 27, 2016

Choose a reason for hiding this comment

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

At least on github the indentation seems to be not correct. ✅


void initPropertyValue(std::size_t group_id, std::vector<T> const& values)
{
T* p = new T[values.size()];
Copy link
Member

@TomFischer TomFischer Sep 27, 2016

Choose a reason for hiding this comment

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

Maybe a check values.size() == _n_components could be inserted? ✅

@@ -155,13 +156,24 @@ friend class Properties;

void initPropertyValue(std::size_t group_id, T const& value)
{
_values[group_id] = new T(value);
T* p = new T[1];
Copy link
Member

@TomFischer TomFischer Sep 27, 2016

Choose a reason for hiding this comment

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

Maybe a check _n_components == 1 could be inserted? ✅

@TomFischer
Copy link
Member

If tests are ok: 👍

Copy link
Collaborator

@chleh chleh left a comment

Choose a reason for hiding this comment

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

I am OK with this PR.
I am only wondering:
Is PropertyVector<T*> used anywhere? If I remember correctly the material group parameter will not be based on this class.
So what is a use-case for the multi-component extension?
And what will be the fate of this class in the medium/long run? Especially since its current implementation cannot store abstract types (am I correct?), nor unique_ptrs.

OGS_FATAL("Single-component version of initPropertyValue() is called "
"for a multi-components PropertyVector<T*>");
T* p = new T[1];
p[0] = value;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note: This will not work if T is an abstract class or T is not copyable. However, it did not work either before.

@norihiro-w
Copy link
Collaborator Author

@chleh As far as I know, PropertyVector<T*> is used only in tests at the moment. I fully agree with your concerns. It should be discussed at some point.

@norihiro-w norihiro-w merged commit 9b57cfd into ufz:master Sep 27, 2016
@norihiro-w norihiro-w deleted the fix-PropertyVecStar-multicomp branch September 27, 2016 08:22
@ogsbot
Copy link
Member

ogsbot commented Jun 19, 2020

OpenGeoSys development has been moved to GitLab.

See this pull request on GitLab.

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

Successfully merging this pull request may close these issues.

5 participants