Bunch of missing `else` statements and general logic #7878

RebelliousX · 2017-02-23T08:39:26Z

Some else statements are missing. Also, some of compound statements of the if-else are equal! I don't know if they are intentional.

RebelliousX · 2017-02-23T08:40:38Z

Please review the changes, I added some comments on some if statements too.

akien-mga · 2017-02-23T08:45:36Z

main/tests/test_math.cpp

@@ -148,7 +148,7 @@ class GetClassAndNamespace {

 									idx+=2;
 									break;
-								} if (code[idx]=='\n') {
+								} else if (code[idx]=='\n') {


I guess the previous if needs an else too.

akien-mga · 2017-02-23T08:46:25Z

modules/visual_script/visual_script_builtin_funcs.cpp

@@ -329,7 +329,7 @@ PropertyInfo VisualScriptBuiltinFunc::get_input_value_port_info(int p_idx) const
 		case LOGIC_CLAMP: {
 			if (p_idx==0)
 				return PropertyInfo(Variant::REAL,"a");
-			else if (p_idx==0)
+			else if (p_idx==0)	// is it ok to test p_idx == 0 twice?


That looks like a bug.

As far as I can tell, the second one should be p_idx==1

Indeed, it should be p_idx==1.

akien-mga · 2017-02-23T08:47:19Z

drivers/gles3/rasterizer_scene_gles3.cpp

@@ -2806,6 +2806,7 @@ void RasterizerSceneGLES3::_copy_to_front_buffer(Environment *env) {
 		//no environment, simply convert from linear to srgb
 		storage->shaders.copy.set_conditional(CopyShaderGLES3::LINEAR_TO_SRGB,true);
 	} else {
+		/* Why are both statements equal? */


Probably a bug. Would be worth checking the similar code in the gles2 rasterizer if it exists (some code was copy pasted and adapted, could have typos), or the git blame.

akien-mga · 2017-02-23T08:48:54Z

scene/gui/item_list.cpp

@@ -314,7 +314,7 @@ void ItemList::move_item(int p_item,int p_to_pos) {

 	if (current<0) {
 		//do none
-	} if (p_item==current) {
+	} else if (p_item==current) {


That actually changes the logic, but I guess the previous behaviour could be considered a bug.

akien-mga · 2017-02-23T08:51:20Z

servers/physics/shape_sw.cpp

@@ -181,7 +181,7 @@ void RayShapeSW::get_supports(const Vector3& p_normal,int p_max,Vector3 *r_suppo
 		r_amount=2;
 		r_supports[0]=Vector3(0,0,0);
 		r_supports[1]=Vector3(0,0,length);
-	} if (p_normal.z>0) {
+	} else if (p_normal.z>0) {


That also changes/fixes a probable logic bug (if p_normal.z was positive, the previous case would always be overridden.

akien-mga · 2017-02-23T08:52:31Z

tools/editor/plugins/texture_region_editor_plugin.cpp

@@ -353,6 +353,7 @@ void TextureRegionEditor::_region_input(const InputEvent& p_input)
 						undo_redo->add_undo_method(atlas_tex.ptr(),"set_region",rect_prev);
 					}
 					else if(node_patch9){
+						/* Is this intentional? */


That's a bug, cc @MarianoGnu.

akien-mga

The changes look good to me, the various comments are also relevant and should likely be discussed and fixed before merging.

RebelliousX · 2017-02-23T10:34:14Z

I added some more, please have a look at them. :)

BTW, I am using PVS-Studio static analyzer free. There is no way in hell I could spot these on my own, no matter how meticulous would I be. There is about a thousand more or less, entry logs as a "potential" bugs.

RebelliousX · 2017-02-23T10:44:02Z

core/variant.cpp

@@ -3065,7 +3065,7 @@ bool Variant::hash_compare(const Variant& p_variant) const {
 				return false;

 			for(int i = 0; i < l.size(); ++i) {
-				if(! l[0].hash_compare(r[0]))
+				if(! l[i].hash_compare(r[i]))


Not sure about this! Do both indices need to be counter i? or just one of them? Are we comparing if the two arrays are equal (it seems to me it is)?

Correct, that is broken. Bad @hpvb, bad.

@RebelliousX Line 2956 has the same bug. I can make a PR for this, or you can roll it in this one.

@hpvb Done ;)

hpvb · 2017-02-23T11:04:41Z

scene/resources/default_theme/default_theme.cpp

@@ -982,7 +982,7 @@ void make_default_theme(bool p_hidpi,Ref<Font> p_font) {
 	Ref<BitmapFont> default_font;
 	if (p_font.is_valid()) {
 		default_font=p_font;
-	} if (p_hidpi) {
+	} else if (p_hidpi) {


I think the bug is elsewhere. With this change the check for p_font.is_valid() becomes useless. If we don't need it remove the whole case.

No I think the change is fine. As I understand it, the behaviour of this method is (at least it was until now and the change keeps it):

If p_font is a valid font, use it

If it isn't, use the default font for either hidpi (if p_hidpi) or lodpi

And indeed that's the expected logic:

godot/scene/register_scene_types.cpp

Lines 284 to 300 in de0045c

String font_path = GLOBAL_DEF("gui/theme/custom_font","");

GlobalConfig::get_singleton()->set_custom_property_info("gui/theme/custom_font",PropertyInfo(Variant::STRING,"gui/theme/custom_font",PROPERTY_HINT_FILE,"*.tres,*.res,*.fnt",PROPERTY_USAGE_DEFAULT|PROPERTY_USAGE_RESTART_IF_CHANGED));

if (theme_path!=String()) {

Ref<Theme> theme = ResourceLoader::load(theme_path);

if (theme.is_valid()) {

Theme::set_default(theme);

}

} else {

Ref<Font> font;

if (font_path!=String()) {

font=ResourceLoader::load(font_path);

}

make_default_theme(default_theme_hidpi,font);

}

Basically the code that calls make_default_theme loads a custom font if one is defined, but does not do the validity check, so it has to happen in make_default_theme (and if no font is provided or if it's invalid, use the default fonts).

Ah, ok, makes sense. I misunderstood the purpose of this code.

akien-mga · 2017-02-26T19:29:34Z

core/io/marshalls.cpp

@@ -641,6 +641,7 @@ Error decode_variant(Variant& r_variant,const uint8_t *p_buffer, int p_len,int *

 			ERR_FAIL_COND_V(len<4,ERR_INVALID_DATA);
 			uint32_t count = decode_uint32(buf);
+			// count always > 0


Is the ERR_FAIL_COND_V test useful at all? count is a uint32_t, so it can't be < 0, right?

Alright, that's what your comment meant. Feel free to remove the checks then.

akien-mga · 2017-02-26T19:30:33Z

core/io/stream_peer.cpp

@@ -492,6 +492,8 @@ Error StreamPeerBuffer::get_partial_data(uint8_t* p_buffer, int p_bytes,int &r_r

 	PoolVector<uint8_t>::Read r = data.read();
 	copymem(p_buffer,r.ptr(),r_received);
+
+	// return what? OK or ERR_*


@Faless An idea maybe?

akien-mga · 2017-02-26T19:30:59Z

core/pool_allocator.cpp

@@ -504,7 +504,7 @@ const void *PoolAllocator::get(ID p_mem) const {
 		return NULL;
 	}

-	if (e->pos<0 || (int)e->pos>=pool_size) {
+	if (e->pos<0 || (int)e->pos>=pool_size) {	// e->pos < 0 is always false, since pos is unsigned


Then remove that test :)

akien-mga · 2017-02-26T19:34:36Z

core/variant_parser.cpp

@@ -1414,7 +1414,7 @@ Error VariantParser::parse_value(Token& token,Variant &value,Stream *p_stream,in
 			return OK;
 		} else if (id=="img") {  // compatibility with godot.cfg

-			Token token;
+			Token token;	// no need for this declaration? the first argument in line 509 is a Token& token.


I guess the idea is that it shadows the other token without modifying its data (while if it modified the Token& reference it would change its data). Whether that's necessary or not, I don't know :)

cc @bojidar-bg @reduz

akien-mga · 2017-02-26T19:36:41Z

modules/regex/regex.cpp

@@ -321,7 +321,7 @@ struct RegExNodeClass : public RegExNode {
 			case Type_lower:
 				return ('a' <= c && c <= 'z');
 			case Type_print:
-				return (0x1F < c && c < 0x1F);
+				return (0x1F < c && c < 0x1F);	// huh ?


Ahhh, it's supposed to be 7F. Also, referring to the POSIX standards again, space is excluded too so it should actually be 0x20 < c && c < 0x7F. 😅

akien-mga · 2017-02-26T19:37:36Z

scene/2d/particles_2d.cpp

@@ -235,7 +235,7 @@ _FORCE_INLINE_ static float _rand_from_seed(uint32_t *seed) {
 		s = 0x12345987;
 	k = s / 127773;
 	s = 16807 * (s - k * 127773) - 2836 * k;
-	if (s < 0)
+	if (s < 0)	// but "s" is unsigned ?


Hm, why do we have this class rolling its own version of rand_from_seed instead of using the Math one?

Math::rand_form_seed() uses a different algorithm. Is it possible that this one was implemented to make sure it looks the same when ran on multiple systems, say for networking? This is just a guess of course.

No, it's because it was replaced a first time in a747c8c, and then a second time in 4c90046. I guess both contributors (cc @tagcup) missed that there was code duplication in this class.

Oh, sorry. Indeed, I didn't even guess there would be multiple PRNG implementations in the code-base. I'll submit a PR ASAP

Submitted PR #7904 for this one

akien-mga · 2017-02-26T19:41:30Z

core/pool_allocator.cpp

@@ -546,7 +546,7 @@ void *PoolAllocator::get(ID p_mem) {
 		return NULL;
 	}

-	if (e->pos<0 || (int)e->pos>=pool_size) {
+	if (e->pos<0 || (int)e->pos>=pool_size) {	// e->pos < 0 is always false, since pos is unsigned


Can be removed.

Oh crap! I forgot to remove another one!

akien-mga · 2017-02-26T19:42:47Z

drivers/gles3/shader_compiler_gles3.cpp

@@ -401,6 +401,7 @@ String ShaderCompilerGLES3::_dump_node_code(SL::Node *p_node, int p_level, Gener
 				String scode = _dump_node_code(bnode->statements[i],p_level,r_gen_code,p_actions,p_default_actions);

 				if (bnode->statements[i]->type==SL::Node::TYPE_CONTROL_FLOW || bnode->statements[i]->type==SL::Node::TYPE_CONTROL_FLOW) {
+					// if (A || A) ? I am hesitant to delete one of them, could be copy-paste error.


akien-mga · 2017-02-26T19:45:00Z

scene/gui/rich_text_label.h

@@ -173,7 +173,7 @@ class RichTextLabel : public Control {

 	struct ItemNewline : public Item {

-		int line;
+		int line;	// Overriding base's line ?


Sounds buggy indeed.

akien-mga · 2017-02-26T19:45:25Z

servers/visual/shader_language.cpp

@@ -3241,7 +3241,7 @@ Error ShaderLanguage::_parse_shader(const Map< StringName, Map<StringName,DataTy
 					_set_error("void datatype not allowed here");
 					return ERR_PARSE_ERROR;
 				}
-				if (!uniform && type<TYPE_FLOAT && type>TYPE_VEC4) {
+				if (!uniform && type<TYPE_FLOAT && type>TYPE_VEC4) {	// always false! should it be || instead?


akien-mga · 2017-02-26T19:45:40Z

tools/editor/import/resource_importer_scene.cpp

@@ -1279,7 +1279,7 @@ Error ResourceImporterScene::import(const String& p_source_file, const String& p
 	Ref<EditorScenePostImport>  post_import_script;

 	if (post_import_script_path!="") {
-		post_import_script_path = post_import_script_path;
+		post_import_script_path = post_import_script_path;	// is there a good reason for this?


hpvb · 2017-02-26T19:46:27Z

core/variant.cpp

@@ -2409,7 +2409,7 @@ Variant::Variant(const Object* p_object) {
 Variant::Variant(const Dictionary& p_dictionary) {

 	type=DICTIONARY;
-	memnew_placement( _data._mem, (Dictionary)( p_dictionary) );
+	memnew_placement( _data._mem, (Dictionary)( p_dictionary) );	// shouldn't there be sizeof() for 2nd argument?


This doesn't need sizeof() the macro expands to :

_post_initialize(new(_data._mem,sizeof((Dictionary)( p_dictionary)),"") (Dictionary)( p_dictionary))

akien-mga · 2017-02-26T19:49:02Z

Nice work overall :) Could you squash the three commits together and edit the commit log to be explicit about the changes?

Also it might be worth adding FIXME: to your comments indicating potential bugs, so that we can still find them if we merged it before every single comment was fixed.

RebelliousX · 2017-02-27T10:32:55Z

@akien-mga I hope I did this right :/ I did interactive rebase and squashed 4 commits.. Is it good? This is the first time doing this :)

hpvb · 2017-02-27T10:36:51Z

@RebelliousX What you want to do is this:

git rebase -i
then hit 'squash' for all but one of your commits
update the commit message to be more useful

Once that is done when you do 'git log' all your old commits should be gone, instead having just the one commit with your new message. If that is what you see do

git push -f <whatever your own fork's upstream is>

If you're on IRC I can help you with this step by step if you'd like.

RebelliousX · 2017-02-27T11:53:03Z

@hpvb Thanks for the offer and the help. I am using Sourcetree on Windows, which is bogus unlike the Mac version. It kept insisting there was an error, apparently, the force push was disabled on the GUI, so I had to use the terminal in the end which was way easier.

akien-mga · 2017-02-27T11:55:24Z

so I had to use the terminal in the end which was way easier

For anything git, the command line is always easier. Git is way too powerful and complex to be usable via a GUI past the basic "drag and drop-box" workflow that GitHub promotes :)

akien-mga · 2017-02-28T09:30:21Z

scene/2d/particles_2d.cpp

@@ -235,7 +235,7 @@ _FORCE_INLINE_ static float _rand_from_seed(uint32_t *seed) {
 		s = 0x12345987;
 	k = s / 127773;
 	s = 16807 * (s - k * 127773) - 2836 * k;
-	if (s < 0)
+	if (s < 0)	// FIXME: but "s" is unsigned ?


This is now fixed in master, so the addition of the comment should be rebased away as it conflicts.

akien-mga · 2017-02-28T09:33:08Z

tools/editor/plugins/texture_region_editor_plugin.cpp

@@ -353,6 +353,7 @@ void TextureRegionEditor::_region_input(const InputEvent& p_input)
 						undo_redo->add_undo_method(atlas_tex.ptr(),"set_region",rect_prev);
 					}
 					else if(node_patch9){
+						// FIXME: Is this intentional?


I think you can just remove that empty else if so that the second one gets triggered.

akien-mga · 2017-02-28T09:40:24Z

servers/visual/shader_language.cpp

@@ -3241,7 +3241,7 @@ Error ShaderLanguage::_parse_shader(const Map< StringName, Map<StringName,DataTy
 					_set_error("void datatype not allowed here");
 					return ERR_PARSE_ERROR;
 				}
-				if (!uniform && type<TYPE_FLOAT && type>TYPE_VEC4) {
+				if (!uniform && type<TYPE_FLOAT && type>TYPE_VEC4) {	// FIXME: always false! should it be || instead?


Yes, it should be ||.

akien-mga · 2017-02-28T09:44:35Z

core/io/stream_peer.cpp

@@ -492,6 +492,8 @@ Error StreamPeerBuffer::get_partial_data(uint8_t* p_buffer, int p_bytes,int &r_r

 	PoolVector<uint8_t>::Read r = data.read();
 	copymem(p_buffer,r.ptr(),r_received);
+
+	// FIXME: return what? OK or ERR_*


It looks like the error handling is actually done in get_data, where it returns ERR_INVALID_PARAMETER if p_bytes != r_received. So maybe this method here should be changed to return void, or the error handling should be added directly here and get_data should just return get_partial_data(p_buffer,p_bytes,recv).

@Faless @punto- Any idea?

I've never used the StreamPeerBuffer class myself but judging from the commit log of cbbcf72 and it's GDScript signature it looks cool.

get_partial_data is inherited from PacketPeer and we cannot change it's type.
It also appears to be exposed to GDScript StreamPeerBuffer.get_partial_data(...) via StreamPeer::_get_partial_data(...).
It think this should return a proper error when needed, so option 2, something like

diff --git a/core/io/stream_peer.cpp b/core/io/stream_peer.cpp index a64ebf2..399fbf9 100644 --- a/core/io/stream_peer.cpp +++ b/core/io/stream_peer.cpp @@ -470,21 +470,20 @@ Error StreamPeerBuffer::put_partial_data(const uint8_t* p_data,int p_bytes, int Error StreamPeerBuffer::get_data(uint8_t* p_buffer, int p_bytes){ int recv; - get_partial_data(p_buffer,p_bytes,recv); - if (recv!=p_bytes) - return ERR_INVALID_PARAMETER; - - return OK; + return get_partial_data(p_buffer,p_bytes,recv); } Error StreamPeerBuffer::get_partial_data(uint8_t* p_buffer, int p_bytes,int &r_received){ + Error err = OK; if (pointer+p_bytes > data.size()) { + err = ERR_FILE_EOF; + r_received=data.size()-pointer; if (r_received<=0) { r_received=0; - return OK; //you got 0 + return err; //you got 0 } } else { r_received=p_bytes; @@ -492,6 +491,7 @@ Error StreamPeerBuffer::get_partial_data(uint8_t* p_buffer, int p_bytes,int &r_r PoolVector<uint8_t>::Read r = data.read(); copymem(p_buffer,r.ptr(),r_received); + return err; } int StreamPeerBuffer::get_available_bytes() const {

akien-mga · 2017-02-28T12:51:50Z

Looks like SourceTree played one of its magic tricks again, you now have three commits instead of one ;)

RebelliousX · 2017-02-28T13:03:57Z

@akien-mga Actually, I used github's resolve conflict feature. I didn't touch Sourcetree till now, but now, it even refuses to do rebase because github merged branches I think :/

- Add FIXME tags comments to some unfixed potential bugs - Remove some checks (always false: unsigned never < 0) - Fix some if statements based on reviews. - Bunch of missing `else` statements

akien-mga · 2017-02-28T22:03:06Z

Let's merge the current state already as it's pretty good. I'll make sure the remaining FIXMEs get fixed soon too.

ghost · 2017-02-28T23:28:46Z

@akien-mga @RebelliousX There are two bugs left unmarked (meaning no recognizable labels such as "FIXME"):

else if (p_idx==0)	// is it ok to test p_idx == 0 twice?

in modules/visual_script/visual_script_builtin_funcs.cpp and

		/* Why are both statements equal? */
  		storage->shaders.copy.set_conditional(CopyShaderGLES3::LINEAR_TO_SRGB,true);

in drivers/gles3/rasterizer_scene_gles3.cpp.

Added missing FIXMEs in PR #7878 [ci skip]

akien-mga reviewed Feb 23, 2017

View reviewed changes

akien-mga approved these changes Feb 23, 2017

View reviewed changes

akien-mga added bug topic:core labels Feb 23, 2017

akien-mga added this to the 3.0 milestone Feb 23, 2017

RebelliousX changed the title ~~Bunch of missing else statements.~~ Bunch of missing else statements and general logic Feb 23, 2017

RebelliousX commented Feb 23, 2017

View reviewed changes

hpvb reviewed Feb 23, 2017

View reviewed changes

akien-mga reviewed Feb 26, 2017

View reviewed changes

hpvb reviewed Feb 26, 2017

View reviewed changes

RebelliousX force-pushed the else branch 2 times, most recently from 6166127 to 2fa2b3b Compare February 27, 2017 11:49

RebelliousX force-pushed the else branch from 2fa2b3b to 9a857ea Compare February 27, 2017 12:29

akien-mga reviewed Feb 28, 2017

View reviewed changes

RebelliousX force-pushed the else branch from 47c5569 to 44efc94 Compare February 28, 2017 13:49

Various fixes detected using PVS-Studio static analyzer.

f50488a

- Add FIXME tags comments to some unfixed potential bugs - Remove some checks (always false: unsigned never < 0) - Fix some if statements based on reviews. - Bunch of missing `else` statements

RebelliousX force-pushed the else branch from 44efc94 to f50488a Compare February 28, 2017 13:52

akien-mga merged commit a1cbe8e into godotengine:master Feb 28, 2017

RebelliousX deleted the else branch March 1, 2017 16:36

akien-mga added a commit that referenced this pull request Mar 2, 2017

Merge pull request #7930 from tagcup/missing_fixme_comments

d7d4bcb

Added missing FIXMEs in PR #7878 [ci skip]

ghost mentioned this pull request Mar 3, 2017

Added missing FIXMEs in PR #7878 #7930

Merged

groscalin pushed a commit to groscalin/godot that referenced this pull request Jul 15, 2017

Added missing FIXMEs in PR godotengine#7878

f55371e

	String font_path = GLOBAL_DEF("gui/theme/custom_font","");
	GlobalConfig::get_singleton()->set_custom_property_info("gui/theme/custom_font",PropertyInfo(Variant::STRING,"gui/theme/custom_font",PROPERTY_HINT_FILE,".tres,.res,*.fnt",PROPERTY_USAGE_DEFAULT\|PROPERTY_USAGE_RESTART_IF_CHANGED));


	if (theme_path!=String()) {
	Ref<Theme> theme = ResourceLoader::load(theme_path);
	if (theme.is_valid()) {
	Theme::set_default(theme);
	}
	} else {

	Ref<Font> font;
	if (font_path!=String()) {
	font=ResourceLoader::load(font_path);
	}
	make_default_theme(default_theme_hidpi,font);
	}

Bunch of missing else statements and general logic #7878

Bunch of missing else statements and general logic #7878

Conversation

RebelliousX commented Feb 23, 2017 • edited Loading

RebelliousX commented Feb 23, 2017

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

akien-mga left a comment

Choose a reason for hiding this comment

RebelliousX commented Feb 23, 2017 • edited Loading

RebelliousX Feb 23, 2017 • edited Loading

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

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

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

akien-mga commented Feb 26, 2017 • edited Loading

RebelliousX commented Feb 27, 2017

hpvb commented Feb 27, 2017

RebelliousX commented Feb 27, 2017

akien-mga commented Feb 27, 2017

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

Faless Feb 28, 2017 • edited Loading

Choose a reason for hiding this comment

akien-mga commented Feb 28, 2017

RebelliousX commented Feb 28, 2017

akien-mga commented Feb 28, 2017

ghost commented Feb 28, 2017

Bunch of missing `else` statements and general logic #7878

Bunch of missing `else` statements and general logic #7878

RebelliousX commented Feb 23, 2017 •

edited

Loading

RebelliousX commented Feb 23, 2017 •

edited

Loading

RebelliousX Feb 23, 2017 •

edited

Loading

akien-mga commented Feb 26, 2017 •

edited

Loading

Faless Feb 28, 2017 •

edited

Loading