Introduce ReadDataContext and WriteDataContext.#1159
Introduce ReadDataContext and WriteDataContext.#1159BenjaminRodenberg merged 26 commits intoprecice:developfrom
Conversation
BenjaminRodenberg
added
the
maintainability
uekerman
left a comment
uekerman
left a comment
There was a problem hiding this comment.
Good refactoring. I think this was needed. Only a few real concerns below.
I am not sure if it is good practice to automatically add a PRECICE_TRACE() call to every function. Makes the code and the debug output harder to read. I always only enable tracing if I could imagine a debug use case.
| const utils::ptr_vector<DataContext> &writeDataContexts() const; | ||
| const std::vector<WriteDataContext *> &writeDataContexts() const; |
There was a problem hiding this comment.
I am not sure about this change. I agree the old one is ugly, but why not a vector of references?
@fsimonis typically has a good opinion on such things.
More similar occasions below.
There was a problem hiding this comment.
IIRC the main reasons for moving from utils::ptr_vector<T> to std::vector<T> were:
- some standard functionality that I needed for the implementation was missing and I did not want to implement it in
utils::ptr_vector, ifstd::vectorcan also do the job. - I do not understand why
utils::ptr_vector<T>actually exists. It looks to me just likestd::vectorwithout a real benefit.
I can also try to get a vector of references working 👍
There was a problem hiding this comment.
I am not sure about this change. I agree the old one is ugly, but why not a vector of references?
References are immutable. Imagine what push_back means if the new element cannot be assigned. So, this doesn't work.
Raw pointers are not a problem as long as they are just used to access the pointees. Raw owning pointers are the real problem.
I do not understand why utils::ptr_vector actually exists.
The utils::ptr_vector<T> is a possibly owning vector of pointers to objects.
I already replaced quite some occurrences with normal vectors.
It looks to me just like std::vector without a real benefit.
It allows to delete all contained elements. That's about it.
@BenjaminRodenberg Maybe we can implement this as std::vector<WriteDataContext> instead? Who owns these Contexts?
…aContext, consequently use references.
Co-authored-by: Benjamin Uekermann <[email protected]>
fsimonis
left a comment
fsimonis
left a comment
There was a problem hiding this comment.
This makes many things clearer and some loops uglier.
Can you explain the difference between a MappingContext, a DataContext, and a MeshContext and how they are related?
Co-authored-by: Frédéric Simonis <[email protected]>
|
@fsimonis can you take another look? Generally I tried fix all the points. Some points are still not 100% clear and I would need some input/help. |
uekerman
left a comment
uekerman
left a comment
There was a problem hiding this comment.
Only looked at my comments again.
3 participants
Main changes of this PR
Introduces
ReadDataContextandWriteDataContextwhich are used for reading/writing data. Both classes are derived fromDataContext.Motivation and additional information
Introducing the two classes reduces direct access to
Mesh::Dataand allows for a clearer separation of both use cases. In #1029 this new design is used to add additional data depending on the context. AWaveform, for example, is only used in theReadDataContextand is not used in theWriteDataContext.Author's checklist
make changelogif there are user-observable changes since the last release. (only visible to developers)make formatto ensure everything is formatted correctly.Reviewers' checklist