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

strategy for usable const& Mesh objects #391

Open
jacobmerson opened this issue May 5, 2022 · 3 comments
Open

strategy for usable const& Mesh objects #391

jacobmerson opened this issue May 5, 2022 · 3 comments

Comments

@jacobmerson
Copy link
Contributor

@ibaned Hoping to get your perspective on the following as a much more experienced developer.

I did a little bit of work trying to fix up the const correctness of some of the ask_ functions in the mesh class but hit a bit of a roadblocks because some of the intermediate data is cached in the mesh tags or other arrays in the mesh class. I was thinking about doing the following if/when I have time to allow a const reference to mesh to be useable.

  1. Create a Cache struct internal to the Mesh class that holds all of the data/tags that is cached.
  2. Make the stored instance of Cache mutable so it can be updated in a const reference to Mesh
  3. In the get_tag functions if the tag name doesn't exist check it against the list of names that were used as internal cache variables and give an error message saying "you are relying on deprecated tags use ask_... instead".
@ibaned
Copy link
Collaborator

ibaned commented May 5, 2022

@jacobmerson is it not more correct to just admit that ask_* can "modify" the mesh (create new tags) if the adjacencies aren't cached yet, and provide separate APIs for "get an adjacency that is already cached (const)"?

One example you can look at is std::map. std::map::operator[] is not const because if the key does not exist it creates it. On the other hand, std::map::find is const because it does not modify the map if the key does not exist.

@jacobmerson
Copy link
Contributor Author

@ibaned If the data mentioned is truly meant to be cached and the intermediate data currently stored in the mesh tags is not intended to be directly access via get_tag (which the ask_ function interfaces imply)

My personal opinion is that making the cache mutable or storing it as a pointer (both options discussed in the c++ core guidelines) gives the cleanest interface and is most obvious about intent.

If you split the interface into:

  1. user derive cached data like adjacencies
  2. user get data from the const mesh object

Now we force the user to manage if the data is cached. And that's not particularly friendly since some functions may invalidate the cache data. So, in the user code you must always first check if the cached data exists, then build it if it doesn't, then get it.

But, I defer to your experience and what you'll accept here.

@ibaned
Copy link
Collaborator

ibaned commented May 10, 2022

Yea, I think we both understand what the dilemma is here. Do we expose const to the user in a pedantic way (you modified the data structure so not const) or in the mesh philosophical way (as long as you don't change connectivity or tag values then you are const). I think it's okay to do the mesh philosophical way, and use mutable for the adjacency cache. If you want to start a PR for this that would be great. Please be sure to include a comment (probably around the mutable cache declaration) explaining why the design is the way that it is.

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