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

Preinitialized variables #2757

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@sphinxc0re
Contributor

sphinxc0re commented Dec 15, 2015

No description provided.

@madmaxoft

This comment has been minimized.

Show comment
Hide comment
@madmaxoft

madmaxoft Dec 16, 2015

Member

Do not merge yet!

All these values should be initialized properly in the CalcFloorFrac() function, yet both clang and coverity complain that they aren't. This needs further investigation, rather than slapping zeroes all over the place. If there is indeed a possibility of uninitialized value getting in there, then we have a real problem.

Member

madmaxoft commented Dec 16, 2015

Do not merge yet!

All these values should be initialized properly in the CalcFloorFrac() function, yet both clang and coverity complain that they aren't. This needs further investigation, rather than slapping zeroes all over the place. If there is indeed a possibility of uninitialized value getting in there, then we have a real problem.

@SafwatHalaby

This comment has been minimized.

Show comment
Hide comment
@SafwatHalaby

SafwatHalaby Dec 16, 2015

Member

A general remark regarding intialization:
I personally dislike initializing variables with placeholder values (like zeros) which will be overridden later before ever being used, because it reduces the likelihood of crashing when there is an error. Failing noisily is a good thing.

It is also a waste of cpu cycles.

Member

SafwatHalaby commented Dec 16, 2015

A general remark regarding intialization:
I personally dislike initializing variables with placeholder values (like zeros) which will be overridden later before ever being used, because it reduces the likelihood of crashing when there is an error. Failing noisily is a good thing.

It is also a waste of cpu cycles.

@tigerw

This comment has been minimized.

Show comment
Hide comment
@tigerw

tigerw Dec 17, 2015

Member

I remember reading somewhere that just passing uninitialised variables can constitute undefined behaviour, but the article concerned C.

On 16 Dec 2015, at 10:24, "Safwat Halaby" notifications@github.com wrote:

A general remark regarding intialization:
I personally dislike initializing variables with placeholder values (like zeros) which will be overridden later before ever being used. Because it reduces the likelihood of crashing when there is an error. Failing noisily is a good thing. It is also a waste of cpu cycles.


Reply to this email directly or view it on GitHub.

Member

tigerw commented Dec 17, 2015

I remember reading somewhere that just passing uninitialised variables can constitute undefined behaviour, but the article concerned C.

On 16 Dec 2015, at 10:24, "Safwat Halaby" notifications@github.com wrote:

A general remark regarding intialization:
I personally dislike initializing variables with placeholder values (like zeros) which will be overridden later before ever being used. Because it reduces the likelihood of crashing when there is an error. Failing noisily is a good thing. It is also a waste of cpu cycles.


Reply to this email directly or view it on GitHub.

@madmaxoft

This comment has been minimized.

Show comment
Hide comment
@madmaxoft

madmaxoft Dec 17, 2015

Member

If you pass them by-value, it is UB. If you pass them by-ref, it's a valid scenario (you want the function to fill them in)

Member

madmaxoft commented Dec 17, 2015

If you pass them by-value, it is UB. If you pass them by-ref, it's a valid scenario (you want the function to fill them in)

@sphinxc0re

This comment has been minimized.

Show comment
Hide comment
@sphinxc0re

sphinxc0re Dec 25, 2015

Contributor

I'm so sorry for all the whitespace deletions. Atom does some nasty stuff to the code.

I removed most of the unnecessary initializations but I found some more places where uninitialized values throw a warning

Contributor

sphinxc0re commented Dec 25, 2015

I'm so sorry for all the whitespace deletions. Atom does some nasty stuff to the code.

I removed most of the unnecessary initializations but I found some more places where uninitialized values throw a warning

@@ -134,7 +134,7 @@ static Json::Value JsonSerializeTable(cLuaState & a_LuaState)
{
if (lua_type(a_LuaState, -2) == LUA_TNUMBER)
{
int idx;
int idx = 0;
a_LuaState.GetStackValue(-2, idx);

This comment has been minimized.

@worktycho

worktycho Dec 25, 2015

Member

This call to GetStackValues should check the return code for an error. Then the uninitialized value would never be read.

@worktycho

worktycho Dec 25, 2015

Member

This call to GetStackValues should check the return code for an error. Then the uninitialized value would never be read.

This comment has been minimized.

@sphinxc0re

sphinxc0re Dec 25, 2015

Contributor

This should be handled in a different PR

@sphinxc0re

sphinxc0re Dec 25, 2015

Contributor

This should be handled in a different PR

This comment has been minimized.

@madmaxoft

madmaxoft Dec 26, 2015

Member

I don't think it's needed, because we've already checked that it's a number, so it's always read successfully (unless someone else bombs our memory).
I guess I could rewrite this to instead depend on the GetStackValue()'s return as to whether the value is an int or a string

@madmaxoft

madmaxoft Dec 26, 2015

Member

I don't think it's needed, because we've already checked that it's a number, so it's always read successfully (unless someone else bombs our memory).
I guess I could rewrite this to instead depend on the GetStackValue()'s return as to whether the value is an int or a string

This comment has been minimized.

@madmaxoft

madmaxoft Dec 26, 2015

Member

Still, another PR.

@madmaxoft

madmaxoft Dec 26, 2015

Member

Still, another PR.

This comment has been minimized.

@worktycho

worktycho Dec 27, 2015

Member

It can fail. Lua numbers are doubles, and this is an int. So if I write plugin code to pass 1e40 to the function, its a number but its too big to fit in an int so get stack values should fail.

@worktycho

worktycho Dec 27, 2015

Member

It can fail. Lua numbers are doubles, and this is an int. So if I write plugin code to pass 1e40 to the function, its a number but its too big to fit in an int so get stack values should fail.

@sphinxc0re

This comment has been minimized.

Show comment
Hide comment
@sphinxc0re

sphinxc0re Dec 27, 2015

Contributor

Merge? @madmaxoft

Contributor

sphinxc0re commented Dec 27, 2015

Merge? @madmaxoft

Julian Laubstein
@sphinxc0re

This comment has been minimized.

Show comment
Hide comment
@sphinxc0re

sphinxc0re Dec 27, 2015

Contributor

Rebased again. Merge?

Contributor

sphinxc0re commented Dec 27, 2015

Rebased again. Merge?

@madmaxoft

This comment has been minimized.

Show comment
Hide comment
@madmaxoft

madmaxoft Dec 28, 2015

Member

I'm against a merge of this kind of code. It hides the problems, instead of solving them.

We need to find out why the variables aren't initialized in the functions that are expected to assign values to them, instead of slapping the "don't care" plaster all over the place.

Also, set your editor not to remove the final empty lines of the files; those unrelated whitespace changes are not nice either.

Member

madmaxoft commented Dec 28, 2015

I'm against a merge of this kind of code. It hides the problems, instead of solving them.

We need to find out why the variables aren't initialized in the functions that are expected to assign values to them, instead of slapping the "don't care" plaster all over the place.

Also, set your editor not to remove the final empty lines of the files; those unrelated whitespace changes are not nice either.

@tigerw tigerw deleted the wrn2g branch Jul 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment