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

Compiler warning generated when using runge_kutta.c #107

Open
yantosca opened this issue Jul 17, 2024 · 1 comment
Open

Compiler warning generated when using runge_kutta.c #107

yantosca opened this issue Jul 17, 2024 · 1 comment
Labels
bug Something isn't working integrators Related to numerical integrators

Comments

@yantosca
Copy link
Contributor

This is a minor issue but I just thought I'd bring it up. When compiling KPP 3.1.1 with gcc 10.2.0, I get this warning:

C_rk_Integrator.c: In function ‘RK_Integrator’:
C_rk_Integrator.c:1045:34: warning: ‘ErrOld’ may be used uninitialized in this function [-Wmaybe-uninitialized]
 1045 |        FacGus = FacSafe*(H/Hacc)*pow(Err*Err/ErrOld,(double)(-0.25));
      |                                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
C_rk_Integrator.c:1045:27: warning: ‘Hacc’ may be used uninitialized in this function [-Wmaybe-uninitialized]
 1045 |        FacGus = FacSafe*(H/Hacc)*pow(Err*Err/ErrOld,(double)(-0.25));
      |                         ~~^~~~~~

Which I've traced to this section of code:

 /*~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~*/
 /*~~~> Accept/reject step */
 /*~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~*/
      // accept:
      if (Err < ONE) { /*~~~> STEP IS ACCEPTED */
      	FirstStep = 0;
      	ISTATUS[Nacc]++;
      	if (Gustafsson == 1) {
	   /*~~~> Predictive controller of Gustafsson */
	   if (ISTATUS[Nacc] > 1) {
	      FacGus = FacSafe*(H/Hacc)*pow(Err*Err/ErrOld,(KPP_REAL)(-0.25));
	      FacGus = MIN(FacMax,MAX(FacMin,FacGus));
	      Fac = MIN(Fac,FacGus);	
	      Hnew = Fac*H;
	   } /* end if */
	   Hacc = H;
	   ErrOld = MAX((KPP_REAL)1.0e-02,Err);
      	} /* end if */

The compiler is flagging that ErrOld may be uninitialized. It doesn't look like it's used until after the first timestep.

Would it be safe to set e.g. ErrOld = Err just after the FirstStep=0 line? That would remove the warning.

Also a similar situation is in the F90 code but we don't get a warning for that.

This isn't an essential fix but would be more of a cleanup so that we wouldn't generate warnings when building the mechanism.

@yantosca yantosca added bug Something isn't working integrators Related to numerical integrators labels Jul 17, 2024
@RolfSander
Copy link
Contributor

The code seems to be written in a way that ErrOld is never
uninitialized: ISTATUS starts with zero. It is then incremented to 1,
meaning that if (ISTATUS[Nacc] > 1) is still false and ErrOld is not
used yet. Anyway, in the spirit of clean coding, I agree that ErrOld
should be initialized with a dummy value.

I don't think that after FirstStep=0 is the right place. To be on the
safe side, I'd put it at the very beginning of the function, i.e.,
before Tdirection = SIGN....

For consistency, I think that the f90 code should be adjusted as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working integrators Related to numerical integrators
Projects
None yet
Development

No branches or pull requests

2 participants