Strict compiler warning policy for CI builds #1203

pcarruscag · 2021-02-22T12:23:42Z

Proposed Changes

Regression tests will fail (code will not compile) on pedantic warnings, i.e. -Wall -Wextra -Wpedantic -Werror
For external libraries many warnings were manually disabled, because I don't know a better way of not having -Werror defined for them via meson.

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.

pcarruscag · 2021-02-22T12:24:50Z

externals/metis/meson.build

+    metis_default_warnings += ['-Wno-unused-result',
+                               '-Wno-unused-parameter',
+                               '-Wno-unused-variable',
+                               '-Wno-unused-but-set-variable',
+                               '-Wno-macro-redefined',
+                               '-Wno-unknown-pragmas',
+                               '-Wno-sign-compare',
+                               '-Wno-clobbered',
+                               '-Wno-empty-body',
+                               '-Wno-unused-label',
+                               '-Wno-misleading-indentation',
+                               '-Wno-pedantic']


maybe compiler warnings had not been invented when metis was written

TobiKattmann

Thanks pedro 💐
On top of the compiler warning changes for CI this PR contains a lot of pointers that became std::vector's, trailing whitespace cleanup and other things.

TobiKattmann · 2021-02-23T15:27:07Z

SU2_CFD/include/solvers/CTurbSolver.hpp


  /*--- Sliding meshes variables. ---*/

-  su2double ****SlidingState = nullptr;
-  int **SlidingStateNodes = nullptr;
+  vector<su2matrix<su2double*> > SlidingState; // vector of matrix of pointers... inner dim alloc'd elsewhere (welcome, to the twilight zone)


:) Note to self: never deal with sliding states

TobiKattmann · 2021-02-23T15:36:55Z

SU2_CFD/include/solvers/CFVMFlowSolverBase.hpp

@@ -2193,7 +2193,7 @@ class CFVMFlowSolverBase : public CSolver {
   * \param[in] val_vertex - Vertex of the marker <i>val_marker</i> where the coefficient is evaluated.
   * \return Value of the pressure coefficient.
   */
-  inline su2double* GetCharacPrimVar(unsigned short val_marker, unsigned long val_vertex) const final {


y remove const 🐒 ? Changing to vector<su2activematrix> CharacPrimVar; would force you to also have a const return type, right? i.e.

inline const su2double* GetCharacPrimVar(unsigned short val_marker, unsigned long val_vertex) const final {

which makes things more complicated down the line

Yes, which is one of the benefits of having container types, you can actually enforce constness of member variables. With pointers you cannot do anything.

TobiKattmann · 2021-02-23T15:41:04Z

SU2_CFD/src/fluid/CNEMOGas.cpp

-  Cvtrs.resize(nSpecies,0.0);         
-  Cvves.resize(nSpecies,0.0);               
-  eves.resize(nSpecies,0.0);            


Somebody should set-up clang-tidy to format all merged PR's to get rid of those things ... I said somebody... not me :D

TobiKattmann · 2021-02-23T15:42:56Z

SU2_CFD/src/output/filewriter/CTecplotBinaryFileWriter.cpp

@@ -135,6 +134,8 @@ void CTecplotBinaryFileWriter::Write_Data(){

 #ifdef HAVE_MPI

+  unsigned long nParallel_Line = dataSorter->GetnElem(LINE);


ok i don't use tecplot (so feel free to ignore) but I am curious what happened here? (Prob found because of the added compiler flags)

me neither, that variable was not used for serial compilation, locally (gcc 7.5) it was not a warning but gcc 7.4 complained

TobiKattmann · 2021-02-23T15:54:37Z

SU2_CFD/src/solvers/CNEMONSSolver.cpp

  su2double C, alpha_V, alpha_T;

  su2double TMAC, TAC;
  su2double Viscosity, Eddy_Visc, Lambda;
  su2double Density, GasConstant;

-  su2double **Grad_PrimVar;
-  su2double Vector_Tangent_dT[3], Vector_Tangent_dTve[3], Vector_Tangent_HF[3];
+  const su2double* const* Grad_PrimVar;


okok
const pointer and const values ... but I wasn't aware that the * are split and one is with the second const which indicates a const pointer. (But I might be completely wrong here)
Did this get necesary with another change in this PR (which I missed), was it a warning?

you could put even one more const... to say that you cannot change what is points to.
It is in my todo list to create a "matrix view" to be able to access the gradient without copying and without pointers... but I will never get there.

If there is time in the Dev meeting tomorrow I might ask you again about this (both the code and your todo-list item) ... because i did not really get it :) 🐌

TurboLdd · 2024-03-02T12:30:47Z

SU2_CFD/include/solvers/CFVMFlowSolverBase.hpp

-  su2double** YPlus = nullptr;             /*!< \brief Yplus for each boundary and vertex. */
-
-  bool space_centered;       /*!< \brief True if space centered scheeme used. */
+  vector<su2double> CNearFieldOF_Inv;    /*!< \brief Near field pressure (inviscid) for each boundary. */


Hi!, I noticed that these codes from line 140 assign every boundary marker a container, even through these boundaries don't need these variable. I'm not sure if I'm understanding this correctly。

If true, I think these codes are wasting memory, and I wonder is there any better way to store these variables?

pedantic warnings for CI builds, disable warnings of external libs

2f9898d

pr-triage bot added the PR: unreviewed label Feb 22, 2021

pcarruscag added the changelog:chore label Feb 22, 2021

pcarruscag commented Feb 22, 2021

View reviewed changes

pcarruscag added 5 commits February 22, 2021 12:29

one more for metis

f20d790

fix some warnings, replace exit with SU2_MPI::Error

daae8ff

fewer bare pointers in flow solver base

9adacff

fix tecplot writer

4502861

no more pointer, except all the turbo stuff

8642dcd

pcarruscag merged commit c863e9c into develop Feb 22, 2021

pcarruscag deleted the strict_warning_policy branch February 22, 2021 23:07

pr-triage bot added PR: merged and removed PR: unreviewed labels Feb 22, 2021

TobiKattmann reviewed Feb 23, 2021

View reviewed changes

pr-triage bot added PR: unreviewed and removed PR: merged labels Feb 23, 2021

TurboLdd reviewed Mar 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Strict compiler warning policy for CI builds #1203

Strict compiler warning policy for CI builds #1203

pcarruscag commented Feb 22, 2021

pcarruscag Feb 22, 2021

TobiKattmann left a comment

TobiKattmann Feb 23, 2021

TobiKattmann Feb 23, 2021

pcarruscag Feb 23, 2021

TobiKattmann Feb 23, 2021

TobiKattmann Feb 23, 2021

pcarruscag Feb 23, 2021

TobiKattmann Feb 23, 2021

pcarruscag Feb 23, 2021

TobiKattmann Feb 23, 2021

TurboLdd Mar 2, 2024

TurboLdd Mar 2, 2024

		@@ -135,6 +134,8 @@ void CTecplotBinaryFileWriter::Write_Data(){

		#ifdef HAVE_MPI

		unsigned long nParallel_Line = dataSorter->GetnElem(LINE);

Strict compiler warning policy for CI builds #1203

Strict compiler warning policy for CI builds #1203

Conversation

pcarruscag commented Feb 22, 2021

Proposed Changes

PR Checklist

Choose a reason for hiding this comment

TobiKattmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment