Cleanup more pointers by pcarruscag · Pull Request #1225 · su2code/SU2

pcarruscag · 2021-03-07T16:23:06Z

Proposed Changes

Pointers are pure evil.

PR Checklist

I am submitting my contribution to the develop branch.
My contribution generates no new compiler warnings (try with the '-Wall -Wextra -Wno-unused-parameter -Wno-empty-body' compiler flags, or simply --warnlevel=2 when using meson).
My contribution is commented and consistent with SU2 style.
I have added a test case that demonstrates my contribution, if necessary.
I have updated appropriate documentation (Tutorials, Docs Page, config_template.cpp) , if necessary.

lgtm-com · 2021-03-07T17:15:43Z

This pull request introduces 2 alerts and fixes 6 when merging 690c98c into b6b2e55 - view on LGTM.com

new alerts:

2 for Multiplication result converted to larger type

fixed alerts:

6 for Comparison of narrow type with wide type in loop condition

lgtm-com · 2021-03-07T19:40:25Z

This pull request introduces 2 alerts and fixes 6 when merging e35a5e2 into b6b2e55 - view on LGTM.com

new alerts:

2 for Multiplication result converted to larger type

fixed alerts:

6 for Comparison of narrow type with wide type in loop condition

…nters

pcarruscag · 2021-03-09T12:41:06Z

Why is it good C++ to use containers (instead of raw arrays):

Book keeping: No need for verbose allocations / de-allocations.
Information: The size is carried with the container, the type of data, the allocation strategy (e.g. alignment), etc.
Correctness: A const method can change the elements of a raw array, because in such cases const does not qualify the data, only the address (pointer) of that data. On the other hand, a const method will imply const containers, on which only const methods can be called, and so on.
Performance 1: A nested array (**), or nested container (vector) is NOT a matrix, it is a list of arrays, consequently there is more construction / destruction overhead (more allocations) uncertain performance at runtime, because inner arrays can end up arbitrarily far from each other, you end up needing copies to talk with e.g. blas or mpi...
Versatility: Above all a container encapsulates data structure details (e.g. if something is a vector or list or map or row-major matrix or column-major matrix or sparse matrix) behind an interface. Which then lets you change the details separately from the code that uses the data. Whenever you pass an array by pointer you are locking yourself to ONE very particular organization of data in memory.
Performance 2: This encapsulation is not insulation, it is completely transparent to the compiler because any half decent container type will be defined inline. Furthermore... If the compiler sees two pointers it has absolutely no idea if they are pointing to the same thing, or worse if they partially overlap. To legally optimize such code compilers need to introduce a ton of checks and fallback code, on the other hand, defining operations on containers which you know never to overlap provides a clean way to avoid compiler-paranoia.

pcarruscag · 2021-03-09T12:51:38Z

"But wait there's more"

Debugging, profiling, logging: The access methods of containers can do bound-checking for debugging, they can be instrumented to measure how much data is used, or to log every imaginable type of information. Raw arrays will have you run to valgrind or address sanitizers and then good luck if the only example where you are able to reproduce a bug is too large and you need to wait one hour per test.

lgtm-com · 2021-03-09T12:54:57Z

This pull request fixes 4 alerts when merging 2d0b349 into 0a219b1 - view on LGTM.com

fixed alerts:

4 for Comparison of narrow type with wide type in loop condition

lgtm-com · 2021-03-09T16:37:13Z

This pull request fixes 4 alerts when merging d4653a1 into 0a219b1 - view on LGTM.com

fixed alerts:

4 for Comparison of narrow type with wide type in loop condition

TobiKattmann

LGTM 💐 and thanks for the passionate plea for using containers instead of pointers. The jury withdraws for its final deliberations 🧑‍⚖️ . Jokes aside, it is really good to get some insights
In order to nag about sth: I would appreciate a few more words in the PR description beyond bullying language features (Not sure what the pointer union has to say about that). Like using config->GetSolid_Wall instead of individual listings in several spaces; RMS_RES & BGS_RES vars now in containers for all(?) solvers (and all necessary work around that). Just some (more) highlights
(And have fun furthermore with deleting 'em all :) )

TobiKattmann · 2021-03-09T16:08:06Z

Common/src/geometry/CGeometry.cpp

-    if ((config->GetMarker_All_KindBC(iMarker) == HEAT_FLUX)               ||
-        (config->GetMarker_All_KindBC(iMarker) == ISOTHERMAL)              ||
-        (config->GetMarker_All_KindBC(iMarker) == EULER_WALL)                )
+    if (config->GetSolid_Wall(iMarker))


👍 good catch (here and elsewhere). I have to admit I don't really understand what this method is doing after briefly looking at it, and grepping the repo indicates that is is unused ...

TobiKattmann · 2021-03-09T16:15:01Z

SU2_CFD/include/solvers/CFVMFlowSolverBase.inl

  SlidingStateNodes.resize(nMarker);

-  for (iMarker = 0; iMarker < nMarker; iMarker++) {
+  for (unsigned long iMarker = 0; iMarker < nMarker; iMarker++) {


any particular reason not to use auto iMarker = 0ul. asking out of curiosity

hmmmm ah yes, I cut and pasted it from the declaration at the top of the function :)

TobiKattmann · 2021-03-09T16:16:45Z

SU2_CFD/include/solvers/CSolver.hpp

+  su2activematrix Point_Max_Coord;     /*!< \brief Vector with pointers to the coords of the maximal residual for each variable. */
+  su2activematrix Point_Max_Coord_BGS; /*!< \brief Vector with pointers to the coords of the maximal residual for each variable. */
+
+  /*--- Variables that need to go. ---*/


:) The pointer-killer already found his next victims. Only a matter of time until he strikes again ...

TobiKattmann · 2021-03-09T16:23:51Z

SU2_CFD/include/solvers/CSolver.hpp

+  inline void SetResToZero() {
+    SU2_OMP_MASTER {
+      for (auto& r : Residual_RMS) r = 0;
+      for (auto& r : Residual_Max) r = 0;


👍 Range-based for loops are a thing in c++ since 11 but I rarely ever used them, whilst in python I use them all the time. I really need to pay more attention when I can use them

There is a type in graph_toolbox.hpp called DummyGridColor with which one can write:

for (const auto iPoint : DummyGridColor<>(nPoint)) { // awesome code }

Should have given it a better name like idk... Range 🤦
Anyway having that const auto iPoint is real useful when we go for them record breaking nested loops 👍

pcarruscag · 2021-03-09T17:38:04Z

SU2_CFD/include/solvers/CSolver.hpp

-  su2double *Residual_RMS,  /*!< \brief Vector with the mean residual for each variable. */
-  *Residual_Max,            /*!< \brief Vector with the maximal residual for each variable. */
-  *Residual,                /*!< \brief Auxiliary nVar vector. */
+  vector<su2double> Residual_RMS;      /*!< \brief Vector with the mean residual for each variable. */
+  vector<su2double> Residual_Max;      /*!< \brief Vector with the maximal residual for each variable. */
+  vector<su2double> Residual_BGS;      /*!< \brief Vector with the mean residual for each variable for BGS subiterations. */


Summary of what this PR is about.

I gotta delete 'em all

690c98c

pcarruscag added the changelog:chore label Mar 7, 2021

pr-triage bot added the PR: unreviewed label Mar 7, 2021

fix issue with multigrid

e35a5e2

pcarruscag added 3 commits March 9, 2021 10:43

Merge branch 'some_adjoint_fixes' into cleanup_more_pointers

b028d98

remove AddRes_RMS and co.

cdac8dd

Merge remote-tracking branch 'upstream/develop' into cleanup_more_poi…

2d0b349

…nters

replace ENUM boundary type logic with Solid/Viscous wall booleans

d4653a1

pcarruscag mentioned this pull request Mar 9, 2021

Heat Transfer boundary condition #1226

Merged

5 tasks

TobiKattmann approved these changes Mar 9, 2021

View reviewed changes

pr-triage bot added PR: reviewed-approved and removed PR: unreviewed labels Mar 9, 2021

pcarruscag commented Mar 9, 2021

View reviewed changes

pcarruscag merged commit b7f6d4f into develop Mar 9, 2021

pcarruscag deleted the cleanup_more_pointers branch March 9, 2021 20:40

pr-triage bot added PR: merged and removed PR: reviewed-approved labels Mar 9, 2021

maxaehle mentioned this pull request Apr 7, 2021

Fix memory leaks in CHeatSolver #1256

Merged

4 tasks

Conversation

pcarruscag commented Mar 7, 2021

Proposed Changes

PR Checklist

Uh oh!

lgtm-com bot commented Mar 7, 2021

Uh oh!

lgtm-com bot commented Mar 7, 2021

Uh oh!

pcarruscag commented Mar 9, 2021

Uh oh!

pcarruscag commented Mar 9, 2021

Uh oh!

lgtm-com bot commented Mar 9, 2021

Uh oh!

lgtm-com bot commented Mar 9, 2021

Uh oh!

TobiKattmann left a comment

Choose a reason for hiding this comment

Uh oh!

TobiKattmann Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

TobiKattmann Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

pcarruscag Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

TobiKattmann Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

TobiKattmann Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

pcarruscag Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

pcarruscag Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants