From f50488a36188d5975bfa8554687a1acdd394d6a9 Mon Sep 17 00:00:00 2001 From: Thaer Razeq Date: Thu, 23 Feb 2017 02:28:09 -0600 Subject: [PATCH] Various fixes detected using PVS-Studio static analyzer. - 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 --- core/io/http_client.cpp | 2 +- core/io/marshalls.cpp | 4 ---- core/io/stream_peer.cpp | 2 ++ core/pool_allocator.cpp | 4 ++-- core/variant.cpp | 4 ++-- core/variant_op.cpp | 4 ++-- core/variant_parser.cpp | 2 +- drivers/gles3/rasterizer_canvas_gles3.cpp | 2 +- drivers/gles3/rasterizer_scene_gles3.cpp | 1 + drivers/gles3/shader_compiler_gles3.cpp | 1 + main/tests/test_math.cpp | 4 ++-- modules/gdscript/gd_function.cpp | 2 +- modules/regex/regex.cpp | 2 +- modules/visual_script/visual_script_builtin_funcs.cpp | 2 +- scene/2d/particles_2d.cpp | 1 + scene/gui/control.cpp | 2 -- scene/gui/dialogs.cpp | 2 +- scene/gui/item_list.cpp | 2 +- scene/gui/rich_text_label.h | 2 +- scene/gui/text_edit.cpp | 2 +- scene/resources/default_theme/default_theme.cpp | 2 +- servers/physics/shape_sw.cpp | 2 +- servers/visual/shader_language.cpp | 4 ++-- tools/editor/collada/collada.cpp | 8 ++++---- tools/editor/import/resource_importer_obj.cpp | 2 +- tools/editor/import/resource_importer_scene.cpp | 2 +- .../editor/plugins/collision_polygon_2d_editor_plugin.cpp | 2 +- tools/editor/plugins/light_occluder_2d_editor_plugin.cpp | 2 +- tools/editor/plugins/navigation_polygon_editor_plugin.cpp | 2 +- tools/editor/plugins/polygon_2d_editor_plugin.cpp | 2 +- tools/editor/plugins/texture_region_editor_plugin.cpp | 1 + tools/editor/property_editor.cpp | 4 +++- 32 files changed, 41 insertions(+), 39 deletions(-) diff --git a/core/io/http_client.cpp b/core/io/http_client.cpp index 5c54f16f13f..ae14f8fa386 100644 --- a/core/io/http_client.cpp +++ b/core/io/http_client.cpp @@ -340,7 +340,7 @@ Error HTTPClient::poll(){ int rs = response_str.size(); if ( (rs>=2 && response_str[rs-2]=='\n' && response_str[rs-1]=='\n') || - (rs>=4 && response_str[rs-4]=='\r' && response_str[rs-3]=='\n' && rs>=4 && response_str[rs-2]=='\r' && response_str[rs-1]=='\n') + (rs>=4 && response_str[rs-4]=='\r' && response_str[rs-3]=='\n' && response_str[rs-2]=='\r' && response_str[rs-1]=='\n') ) { diff --git a/core/io/marshalls.cpp b/core/io/marshalls.cpp index e958edc93e9..a8c1cd0a10e 100644 --- a/core/io/marshalls.cpp +++ b/core/io/marshalls.cpp @@ -641,7 +641,6 @@ 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); - ERR_FAIL_COND_V(count<0,ERR_INVALID_DATA); PoolVector strings; buf+=4; @@ -691,7 +690,6 @@ 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); - ERR_FAIL_COND_V(count<0,ERR_INVALID_DATA); buf+=4; len-=4; @@ -729,7 +727,6 @@ 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); - ERR_FAIL_COND_V(count<0,ERR_INVALID_DATA); buf+=4; len-=4; @@ -768,7 +765,6 @@ 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); - ERR_FAIL_COND_V(count<0,ERR_INVALID_DATA); buf+=4; len-=4; diff --git a/core/io/stream_peer.cpp b/core/io/stream_peer.cpp index a64ebf21fb1..3c4719269f7 100644 --- a/core/io/stream_peer.cpp +++ b/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::Read r = data.read(); copymem(p_buffer,r.ptr(),r_received); + + // FIXME: return what? OK or ERR_* } int StreamPeerBuffer::get_available_bytes() const { diff --git a/core/pool_allocator.cpp b/core/pool_allocator.cpp index b1417dd107a..3260225ac36 100644 --- a/core/pool_allocator.cpp +++ b/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 ((int)e->pos>=pool_size) { mt_unlock(); ERR_PRINT("e->pos<0 || e->pos>=pool_size"); @@ -546,7 +546,7 @@ void *PoolAllocator::get(ID p_mem) { return NULL; } - if (e->pos<0 || (int)e->pos>=pool_size) { + if ((int)e->pos>=pool_size) { mt_unlock(); ERR_PRINT("e->pos<0 || e->pos>=pool_size"); diff --git a/core/variant.cpp b/core/variant.cpp index 132d7a1c882..f45c3fd55df 100644 --- a/core/variant.cpp +++ b/core/variant.cpp @@ -2953,7 +2953,7 @@ uint32_t Variant::hash() const { PoolVector::Read rr = r.read(); \ \ for(int i = 0; i < l.size(); ++i) { \ - if(! p_compare_func((lr[0]), (rr[0]))) \ + if(! p_compare_func((lr[i]), (rr[i]))) \ return false; \ }\ \ @@ -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])) return false; } diff --git a/core/variant_op.cpp b/core/variant_op.cpp index 6ed8a3dd85d..28e804b5bf4 100644 --- a/core/variant_op.cpp +++ b/core/variant_op.cpp @@ -1393,7 +1393,7 @@ void Variant::set(const Variant& p_index, const Variant& p_value, bool *r_valid) v->basis.set_axis(index,p_value); return; } - } if (p_index.get_type()==Variant::STRING) { + } else if (p_index.get_type()==Variant::STRING) { Transform *v=_data._transform; const String *str=reinterpret_cast(p_index._data._mem); @@ -2150,7 +2150,7 @@ Variant Variant::get(const Variant& p_index, bool *r_valid) const { valid=true; return index==3?v->origin:v->basis.get_axis(index); } - } if (p_index.get_type()==Variant::STRING) { + } else if (p_index.get_type()==Variant::STRING) { const Transform *v=_data._transform; const String *str=reinterpret_cast(p_index._data._mem); diff --git a/core/variant_parser.cpp b/core/variant_parser.cpp index 3507501f27c..a2ecb1516df 100644 --- a/core/variant_parser.cpp +++ b/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; // FIXME: no need for this declaration? the first argument in line 509 is a Token& token. get_token(p_stream,token,line,r_err_str); if (token.type!=TK_PARENTHESIS_OPEN) { r_err_str="Expected '(' in old-style godot.cfg construct"; diff --git a/drivers/gles3/rasterizer_canvas_gles3.cpp b/drivers/gles3/rasterizer_canvas_gles3.cpp index 5980a645fa9..9f44d055807 100644 --- a/drivers/gles3/rasterizer_canvas_gles3.cpp +++ b/drivers/gles3/rasterizer_canvas_gles3.cpp @@ -1010,7 +1010,7 @@ void RasterizerCanvasGLES3::canvas_render_items(Item *p_item_list,int p_z,const if (unshaded || (state.canvas_item_modulate.a>0.001 && (!shader_cache || shader_cache->canvas_item.light_mode!=RasterizerStorageGLES3::Shader::CanvasItem::LIGHT_MODE_LIGHT_ONLY) && !ci->light_masked )) _canvas_item_render_commands(ci,current_clip,reclip); - if ((blend_mode==RasterizerStorageGLES3::Shader::CanvasItem::BLEND_MODE_MIX || RasterizerStorageGLES3::Shader::CanvasItem::BLEND_MODE_PMALPHA) && p_light && !unshaded) { + if ((blend_mode==RasterizerStorageGLES3::Shader::CanvasItem::BLEND_MODE_MIX || blend_mode==RasterizerStorageGLES3::Shader::CanvasItem::BLEND_MODE_PMALPHA) && p_light && !unshaded) { Light *light = p_light; bool light_used=false; diff --git a/drivers/gles3/rasterizer_scene_gles3.cpp b/drivers/gles3/rasterizer_scene_gles3.cpp index 87a70d2750b..73adeaf723a 100644 --- a/drivers/gles3/rasterizer_scene_gles3.cpp +++ b/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? */ storage->shaders.copy.set_conditional(CopyShaderGLES3::LINEAR_TO_SRGB,true); } diff --git a/drivers/gles3/shader_compiler_gles3.cpp b/drivers/gles3/shader_compiler_gles3.cpp index 12aac799120..14764afdbaa 100644 --- a/drivers/gles3/shader_compiler_gles3.cpp +++ b/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) { + // FIXME: if (A || A) ? I am hesitant to delete one of them, could be copy-paste error. code+=scode; //use directly } else { code+=_mktab(p_level)+scode+";\n"; diff --git a/main/tests/test_math.cpp b/main/tests/test_math.cpp index 89513b81d95..bbbb903e5f8 100644 --- a/main/tests/test_math.cpp +++ b/main/tests/test_math.cpp @@ -144,11 +144,11 @@ class GetClassAndNamespace { error_str="Unterminated comment"; error=true; return TK_ERROR; - } if (code[idx]=='*' &&code[idx+1]=='/') { + } else if (code[idx]=='*' &&code[idx+1]=='/') { idx+=2; break; - } if (code[idx]=='\n') { + } else if (code[idx]=='\n') { line++; } diff --git a/modules/gdscript/gd_function.cpp b/modules/gdscript/gd_function.cpp index 31bac2748a8..0c72f6d1871 100644 --- a/modules/gdscript/gd_function.cpp +++ b/modules/gdscript/gd_function.cpp @@ -725,7 +725,7 @@ Variant GDFunction::call(GDInstance *p_instance, const Variant **p_args, int p_a err.argument-=1; } } - } if (methodstr=="free") { + } else if (methodstr=="free") { if (err.error==Variant::CallError::CALL_ERROR_INVALID_METHOD) { diff --git a/modules/regex/regex.cpp b/modules/regex/regex.cpp index 6d80532110e..3d1dd048a80 100644 --- a/modules/regex/regex.cpp +++ b/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 (0x20 < c && c < 0x7f); case Type_punct: return (REGEX_NODE_PUNCT.find(c) >= 0); case Type_space: diff --git a/modules/visual_script/visual_script_builtin_funcs.cpp b/modules/visual_script/visual_script_builtin_funcs.cpp index cae0146aac2..1acd5bff8dc 100644 --- a/modules/visual_script/visual_script_builtin_funcs.cpp +++ b/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? return PropertyInfo(Variant::REAL,"min"); else return PropertyInfo(Variant::REAL,"max"); diff --git a/scene/2d/particles_2d.cpp b/scene/2d/particles_2d.cpp index 2b39fefe035..77ace0348aa 100644 --- a/scene/2d/particles_2d.cpp +++ b/scene/2d/particles_2d.cpp @@ -229,6 +229,7 @@ ParticleAttractor2D::ParticleAttractor2D() { /****************************************/ _FORCE_INLINE_ static float _rand_from_seed(uint64_t *seed) { + uint32_t r = Math::rand_from_seed(seed); return 2.0f * (float)r / (float)Math::RANDOM_MAX - 1.0f; } diff --git a/scene/gui/control.cpp b/scene/gui/control.cpp index cffe2ce2181..4161725ad57 100644 --- a/scene/gui/control.cpp +++ b/scene/gui/control.cpp @@ -2421,8 +2421,6 @@ void Control::get_argument_options(const StringName& p_function,int p_idx,Listget_font_list(get_class(),&sn); } else if (pf=="add_constant_override" || pf=="has_constant" || pf=="has_constant_override" || pf=="get_constant") { Theme::get_default()->get_constant_list(get_class(),&sn); - } else if (pf=="add_color_override" || pf=="has_color" || pf=="has_color_override" || pf=="get_color") { - Theme::get_default()->get_color_list(get_class(),&sn); } sn.sort_custom(); diff --git a/scene/gui/dialogs.cpp b/scene/gui/dialogs.cpp index 72d3f0e8f82..e08d933e037 100644 --- a/scene/gui/dialogs.cpp +++ b/scene/gui/dialogs.cpp @@ -201,7 +201,7 @@ void AcceptDialog::_notification(int p_what) { if (p_what==NOTIFICATION_MODAL_CLOSE) { cancel_pressed(); - } if (p_what==NOTIFICATION_RESIZED) { + } else if (p_what==NOTIFICATION_RESIZED) { _update_child_rects(); } diff --git a/scene/gui/item_list.cpp b/scene/gui/item_list.cpp index a35df53e526..91bd16ee0bd 100644 --- a/scene/gui/item_list.cpp +++ b/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) { current=p_to_pos; } else if (p_to_pos>p_item && current>p_item && current p_font) { Ref default_font; if (p_font.is_valid()) { default_font=p_font; - } if (p_hidpi) { + } else if (p_hidpi) { default_font=make_font2(_hidpi_font_height,_hidpi_font_ascent,_hidpi_font_charcount,&_hidpi_font_charrects[0][0],_hidpi_font_kerning_pair_count,&_hidpi_font_kerning_pairs[0][0],_hidpi_font_img_width,_hidpi_font_img_height,_hidpi_font_img_data); } else { default_font=make_font2(_lodpi_font_height,_lodpi_font_ascent,_lodpi_font_charcount,&_lodpi_font_charrects[0][0],_lodpi_font_kerning_pair_count,&_lodpi_font_kerning_pairs[0][0],_lodpi_font_img_width,_lodpi_font_img_height,_lodpi_font_img_data); diff --git a/servers/physics/shape_sw.cpp b/servers/physics/shape_sw.cpp index 10500a306fe..dec1c75d9f5 100644 --- a/servers/physics/shape_sw.cpp +++ b/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) { r_amount=1; *r_supports=Vector3(0,0,length); } else { diff --git a/servers/visual/shader_language.cpp b/servers/visual/shader_language.cpp index 7dfd9822f7b..c680013efa5 100644 --- a/servers/visual/shader_language.cpp +++ b/servers/visual/shader_language.cpp @@ -307,7 +307,7 @@ ShaderLanguage::Token ShaderLanguage::_get_token() { } if (GETCHAR(0)=='*' && GETCHAR(1)=='/') { char_idx+=2; break; - } if (GETCHAR(0)=='\n') { + } else if (GETCHAR(0)=='\n') { tk_line++; } @@ -3241,7 +3241,7 @@ Error ShaderLanguage::_parse_shader(const Map< StringName, MapTYPE_VEC4) { + if (!uniform && typeTYPE_VEC4) { // FIXME: always false! should it be || instead? _set_error("Invalid type for varying, only float,vec2,vec3,vec4 allowed."); return ERR_PARSE_ERROR; } diff --git a/tools/editor/collada/collada.cpp b/tools/editor/collada/collada.cpp index a23fd84aa0c..2ba4f648a32 100644 --- a/tools/editor/collada/collada.cpp +++ b/tools/editor/collada/collada.cpp @@ -352,7 +352,7 @@ void Collada::_parse_image(XMLParser& parser) { image.path=path; - } if (name=="data") { + } else if (name=="data") { ERR_PRINT("COLLADA Embedded image data not supported!"); @@ -728,7 +728,7 @@ void Collada::_parse_effect_material(XMLParser& parser,Effect &effect,String &id #endif } - } if (parser.get_node_type() == XMLParser::NODE_ELEMENT_END && ( + } else if (parser.get_node_type() == XMLParser::NODE_ELEMENT_END && ( parser.get_node_name()=="constant" || parser.get_node_name()=="lambert" || parser.get_node_name()=="phong" || @@ -1100,7 +1100,7 @@ void Collada::_parse_mesh_geometry(XMLParser& parser,String p_id,String p_name) current_source=id; COLLADA_PRINT("source data: "+id); - } else if (section=="float_array" || section=="array" || section=="float_array") { + } else if (section=="float_array" || section=="array") { // create a new array and read it. if (meshdata.sources.has(current_source)) { @@ -2192,7 +2192,7 @@ void Collada::_parse_scene(XMLParser& parser) { state.root_visual_scene=_uri_to_id(parser.get_attribute_value("url")); print_line("***ROOT VISUAL SCENE: "+state.root_visual_scene); - } if (name=="instance_physics_scene") { + } else if (name=="instance_physics_scene") { state.root_physics_scene=_uri_to_id(parser.get_attribute_value("url")); diff --git a/tools/editor/import/resource_importer_obj.cpp b/tools/editor/import/resource_importer_obj.cpp index e6e23366f68..aacb5fbb2d8 100644 --- a/tools/editor/import/resource_importer_obj.cpp +++ b/tools/editor/import/resource_importer_obj.cpp @@ -118,7 +118,7 @@ Error ResourceImporterOBJ::import(const String& p_source_file, const String& p_s nrm.y=v[2].to_float(); nrm.z=v[3].to_float(); normals.push_back(nrm); - } if (l.begins_with("f ")) { + } else if (l.begins_with("f ")) { //vertex has_index_data=true; diff --git a/tools/editor/import/resource_importer_scene.cpp b/tools/editor/import/resource_importer_scene.cpp index ae840e9e16a..406058c59ea 100644 --- a/tools/editor/import/resource_importer_scene.cpp +++ b/tools/editor/import/resource_importer_scene.cpp @@ -1279,7 +1279,7 @@ Error ResourceImporterScene::import(const String& p_source_file, const String& p Ref post_import_script; if (post_import_script_path!="") { - post_import_script_path = post_import_script_path; + post_import_script_path = post_import_script_path; // FIXME: is there a good reason for this? Ref