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

Fix warning C4701: "potentially uninitialized local variable used" #1181

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

fsb4000
Copy link

@fsb4000 fsb4000 commented Aug 16, 2024

Hello.
I tried to use your library (1.86.0) and I faced with an issue. I didn't have the issue with 1.85.0.
I use VS 2022 and I get an error.

boost-math\include\boost\math\special_functions\detail\bessel_ik.hpp(417) : error C2220: the following warning is treated as an error
boost-math\include\boost\math\special_functions\detail\bessel_ik.hpp(417) : warning C4701: potentially uninitialized local variable 'n' used
boost-math\include\boost\math\special_functions\detail\bessel_ik.hpp(417) : warning C4701: potentially uninitialized local variable 'u' used

As you can see we initialize n and u only in the else branch and later we use them in if (reflect). We might not get into this if if we haven't been in that else but the compiler don't know about it. I tried to fix the issue.

@ckormanyos
Copy link
Member

Cool find. I'll try to maybe find/add a test case that hits the lines?

@ckormanyos
Copy link
Member

Cool find. I'll try to maybe find/add a test case that hits the lines?

Wait a sec, ...

According to the covrage report, the lines are being hit. Is there maybe a better fix? Delay declaration of the variables or static_cast<void>?

@jzmaddock
Copy link
Collaborator

I would need to double check, but I seem to remember fixing this while doing some code-coverage work. Ah, yes it is, see: https://github.com/boostorg/math/blob/develop/include/boost/math/special_functions/detail/bessel_ik.hpp

@ckormanyos
Copy link
Member

I think you can declare n and u at lines 340'and 341? Isn't that the point of the warning?

@fsb4000
Copy link
Author

fsb4000 commented Aug 16, 2024

I think you can declare n and u at lines 340'and 341? Isn't that the point of the warning?

They are used not only in the else but later in another if.

else
{
n = iround(v, pol);
u = v - n; // -1/2 <= u < 1/2

if (reflect)
{
T z = (u + n % 2);

Ok, I will try to create a test case...

@fsb4000
Copy link
Author

fsb4000 commented Aug 16, 2024

I found a test case for that:

boost::math::cyl_bessel_k(-1.25, std::numeric_limits<double>::infinity());

Where can I find what result should be for this?
I added

std::cout << "v = " << v<< " x = " << x << " kind = " << kind << " reflect = " << reflect <<std::endl;
std::cout << "u = " << u << " n = " << n << std::endl;

before T z = (u + n % 2); and I got(for version without my changes):

v = 1.25 x = inf kind = 2 reflect = 1
u = 3.18226e-312 n = 4147116544 // I belive they are not initialized.

the call result was 0
What is the real result should be?
Where do you add warning options? Sorry I'm not experienced with b2.

@fsb4000
Copy link
Author

fsb4000 commented Aug 16, 2024

I found a way to fail for that case( BOOST_MATH_ASSERT(fabs(v - n - u) < tools::forth_root_epsilon<T>());). And I think I found a more correct fix: calculate n and u before the if.
I would also add the warning C4701 to your CI(and treat the warning as an error) but I don't know what to add to https://github.com/boostorg/math/blob/develop/.github/workflows/ci.yml for toolset: [ msvc-14.2 ]

@jzmaddock
Copy link
Collaborator

My bad, you are correct that this is NOT fixed in develop, but IS in this branch/PR: #1111 that is still at least somewhat work in progress. I would prefer not to create two competing fixes if possible, as the coverage test branch will be merged fairly soon anyway. Please leave this open and I'll take a look at your test case once I get some time (which will be a couple of weeks).

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

Successfully merging this pull request may close these issues.

3 participants