docs: Adding Code rules page #3915
Conversation
MelReyCG
requested review from
CusiniM,
OmarDuran,
castelletto1,
dkachuma,
herve-gross,
jhuang2601,
paveltomin and
rrsettgast
as code owners
jafranc
left a comment
jafranc
left a comment
There was a problem hiding this comment.
Looks great ! Maybe there is the work of 2 docs there (or more 😄 )
| Use GEOS tensor types for geometric and mechanical calculations: | ||
|
|
||
| .. list-table:: GEOS Tensor Types | ||
| :header-rows: 1 | ||
| :widths: 30 70 | ||
|
|
||
| * - Type | ||
| - Description | ||
| * - ``R1Tensor`` | ||
| - 3D vector (real64) | ||
| * - ``R1Tensor32`` | ||
| - 3D vector (real32) | ||
| * - ``R2SymTensor`` | ||
| - Symmetric 6-component tensor (Voigt notation) |
There was a problem hiding this comment.
I feel like we can expand a bit on why tensor are in use and not their vector counter part
There was a problem hiding this comment.
Do you have a suggestion? I have an idea but I think not precise enough.
There was a problem hiding this comment.
Added a reference to wikipedia article, as we discussed of it in person, I suppose it is resolved.
| Additional Validation Guidelines | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
|
|
||
| - Use ``setInputFlag()`` to mark parameters as ``REQUIRED`` or ``OPTIONAL`` | ||
| - Use ``setApplyDefaultValue()`` to provide sensible defaults | ||
| - Use ``setRTTypeName()`` for runtime type validation (e.g., ``rtTypes::CustomTypes::positive``) | ||
| - Document valid ranges in ``setDescription()`` |
There was a problem hiding this comment.
Shouldn't it be in the Wrapper section ?
There was a problem hiding this comment.
It feels strange to me, as the wrapper section is for documentation requirement. My goal here is to precise validation rules. What do you think?
| Memory Management Rules | ||
| --------------------------------- | ||
|
|
||
| Generalities | ||
| ^^^^^^^^^^^^^^^^^^^^ | ||
|
|
||
| - **Minimize dynamic memory allocation** as much as possible, particularly in performance-critical code, | ||
| - For frequently allocated/deallocated objects, **consider memory pools**, | ||
| - For containers with known size, **reserve capacity upfront**. | ||
|
|
||
| Be Const / Constexpr | ||
| ^^^^^^^^^^^^^^^^^^^^ | ||
|
|
||
| **Use ``const`` and ``constexpr`` when possible**, enabling: | ||
|
|
||
| - **Compiler optimization,** enables better code generation by giving constraints to the code, | ||
| - **Code safety,** prevents accidental modification for constant contexts, | ||
| - **Show code intention,** make code more readable. | ||
|
|
||
| Also, **Mark methods const if the method is not designed to modify** the object state. | ||
|
|
There was a problem hiding this comment.
Same as above, it sounds like very high level advises, maybe better to relocate in another doc ?
There was a problem hiding this comment.
I understand your feeling, and I think the same, strictly forbid useless mutability is a choice, thus a rule.
As it is a rule, it can give strict guidelines when reviewing code.
It is indeed a quite common one, but not enforced in all C++ applications.
I could put the reasons of rules in a foldable block to improve document readability.
I stay open to your feedback, do still you think that it should be moved?
There was a problem hiding this comment.
I think the document could be two documents or main parts.
- One high level C++ do/don't we choose and want to enforce (not sure it is doable).
- One other that is more Domain Specific because GEOS's is using RAJA+CHAI and macros, and this is design/interface
There was a problem hiding this comment.
What do you think of restructuring as follow:
- Code Rules
- Coding Practices
- memory
- perf
- validation / precision
- architecture
- GEOS Features
- typing
- documentation
- testing
- logging
- error managment
- parallelism
- perf
- physics-specific
- Coding Practices
There was a problem hiding this comment.
I agree this'd be a good splitting 👍
|
Thanks @jafranc for your feedback, I'll take them into account. |
|
|
||
| The rule is generalizable to ``string_view`` for strings, but not applicable in GPU context. | ||
|
|
||
| Why Use Views? |
There was a problem hiding this comment.
| Why Use Views? | |
| **Rationale:** | |
There was a problem hiding this comment.
Isn't that less meaningful?
There was a problem hiding this comment.
I do not love "Rationale" but it has been used before in the document. I just put it here again for consistency. Feel free to revert, but then keep it consistent throughout.
There was a problem hiding this comment.
I added "Why?" dropdown sections for all of those, it is now consistent.
|
@jafranc @rrsettgast the rule page is now separated in two and correctly linked in the indexes |
GEOS Code Rules
Aims at writing down the coding standards for GEOS, focusing on type safety, error handling, parallelism, and performance. Key principles include:
These rules ensure code quality, consistency, and maintainability across the GEOS codebase.