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

Rename (or remove) "t8_d*_bits.*" files to "t8_default_*_impl.*" #1340

Open
holke opened this issue Jan 15, 2025 · 2 comments
Open

Rename (or remove) "t8_d*_bits.*" files to "t8_default_*_impl.*" #1340

holke opened this issue Jan 15, 2025 · 2 comments

Comments

@holke
Copy link
Collaborator

holke commented Jan 15, 2025

Feature request

Is your feature request related to a problem? Please describe.

In the default scheme many elements have the two files "t8_dELEMENT_bits.c" and ".h".
For example "t8_dpyramid_bits.c".
The "d" is short for "default".
There is no need to shorten the file name and this shortening only increases the probability for errors and confusion, such as #1339 where we have a serious data type conversion bug.

"bits" is also misleading since the algorithms are mostly not doing any bit manipulation (as was the first intention of that file name).

The names should be changed to "t8_default_ELEMENT_impl.c" and "h"

When we are at it, we could also directly change the endings to "cxx" and "hxx" making the files proper C++ files.

Describe the solution or feature you'd like

Edit: If easily possible (not for all element shapes) remove the files entirely and move the implementation into the existing class implementation of the scheme.

The names should be changed to "t8_default_ELEMENT_impl.c" and "h" (or "*_implementation.c")

When we are at it, we could also directly change the endings to "cxx" and "hxx" making the files proper C++ files.

Describe alternatives you've considered

"impl" or "implementation" are both fine (but the latter does no shortening).

Estimated priority

"Priority: low" Should be solved eventually

Additional context
Add any other context or screenshots about the feature request here.

@Davknapp
Copy link
Collaborator

Wouldn't it also be possible to remove these files entirely and do the implementation directly in t8_default_element.cxx.
That way we would reduce function calls. The only downside that I see is that we would need to cast the elements at the beginning of each function, but it would make thinks a lot shorter and clear the code up.

@holke
Copy link
Collaborator Author

holke commented Jan 16, 2025

Yes that would make sense 👍

However, for tris and tets we use macros to switch between 2D and 3D in order to avoid duplicate code.
I believe to do this properly we would need to switch to a dimension templated C++ implementation of the tri/tet scheme?
(Which @lukasdreyer does for his scheme as i believe?)

So, removing these files may come with additional work and renaming would be a first step.

I edited the Issue description accordingly.

@holke holke changed the title Rename "t8_d*_bits.*" files to "t8_default_*_impl.*" Rename (or remove) "t8_d*_bits.*" files to "t8_default_*_impl.*" Jan 16, 2025
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