Fix Visual Studio Code analysis warnings + some other issues #2181
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@JeromeMartinez Is it supposed to be a fall-through or a break here? MediaInfoLib/Source/MediaInfo/Multiple/File_Mpeg4_Elements.cpp Lines 4407 to 4408 in 04880aa |
A break. |
JeromeMartinez
requested changes
Jan 30, 2025
This PR just resolves all errors in the two files mentioned since I touched those 2 files recently. There are still around a thousand warnings in other files including things like the divide by zero reported in an issue last year. The warning level is not set to level 3 (VS IDE default) or 4 in the project so many things are only shown if running code analysis or by intelisense for current open file. Since not set, it default to level 1. |
For `MediaInfo_Inform` and `File_Mpeg4_Elements` only.
But I prefer to have a PR resolving the same issue everywhere, as we are there...
I am interested in PR for fixing that if you are motivated ;-). |
Some issues difficult for me to fix since I do not know what exactly the code does in some areas and have no files to test for regressions. I can try fix some in my free time to reduce the warnings so that major ones don't get drowned in the warning list. |
I will do QA before merging so no risk about regression.
Appreciated. |
|
Visual Studio warnings remaining: Can fix the CLI one first since only two, then it'll be warning free :) The return value for
49 cases of unannotated fallthrough.... this will take some time. |
Ignored, it is just "best effort" and user will handle with how the terminal is configured by default. But also it would never happen :).
not our code, not to be touched. Should be suggested upstream if you are super motivated.
Oops. |
|
In two files, I left a blank line because I do not know what to put there. Hope I did not make any mistakes with the rest. |
cjee21
changed the title
JeromeMartinez
requested changes
Jan 30, 2025
| @@ -219,6 +219,7 @@ Ztring Mpeg7_ContentCS_Name(int32u termID, MediaInfo_Internal &MI, size_t) //xxy | |||
| case 1 : return __T("Image"); | |||
| case 2 : return __T("Video"); | |||
| case 3 : return __T("Graphics"); | |||
|
|
|||
There was a problem hiding this comment.
"break" after the "}" and move of the content of default below (default can then be removed) after the switch so any unknown value uses this default.
Source/MediaInfo/File__Analyze.cpp
Outdated
| @@ -1829,6 +1829,7 @@ size_t File__Analyze::Read_Buffer_Seek_OneFramePerFile (size_t Method, int64u Va | |||
| if (Config->Demux_Rate_Get()==0) | |||
| return (size_t)-1; //Not supported | |||
| Value=float64_int64s(((float64)Value)/1000000000*Config->Demux_Rate_Get()); | |||
|
|
|||
cjee21
changed the title
|
Breakdown of MediaInfoLib warnings: |
|
Why are these repeated? MediaInfoLib/Source/MediaInfo/Audio/File_Adm.cpp Line 5485 in 04880aa MediaInfoLib/Source/MediaInfo/Video/File_Avc.cpp Line 3163 in 04880aa |
|
MediaInfoLib/Source/MediaInfo/Audio/File_Adm.cpp Line 5485 in 04880aa typo typo second HI_D_Text --> VI_D_Text && --> || MediaInfoLib/Source/MediaInfo/Video/File_Avc.cpp Line 3163 in 04880aa typo |
Both sides are |
FSC_WasNotSet_Sum || FSC_WasSet_Sum |
Source/MediaInfo/Audio/File_Adm.cpp
Outdated
| @@ -2836,8 +2836,8 @@ static void CheckErrors_formatLabelDefinition(file_adm_private* File_Adm_Private | |||
| const auto& Definition = Item.Attributes[label_Info.Definition_Pos]; | |||
| const auto Style = label_Info.Label_Style; | |||
| const auto& Style_Info = Style_Infos[Style]; | |||
| const auto List = (Style == (style)-1) ? nullptr : Style_Infos[Style].formatDefinition_List; | |||
| const unsigned long List_Size = (Style == (style)-1) ? 0 : Style_Infos[Style].formatDefinition_Size; | |||
| const auto List = (Style & (style)-1) ? nullptr : Style_Infos[Style].formatDefinition_List; | |||
There was a problem hiding this comment.
It seems that it is wrongly seen as a power of two but the enum is:
enum style : int8u {
Style_Format,
Style_Type,
Style_Max = (style)-1
};
a third style would be 3 so no more powers of 2.
this warning here is weird because Style_Max is not a power of 2, I don't see how to remove the warning.
Hmmm... Actually a better code could be to remove Style_Max and (style)-1, and to put Style_None as the first element of the list.
checks would become (Style == Style_None) and the compiler will convert to "Style" , micro-optimization.
|
I think that is all I can/will do. Things remaining that I do not know what to do about: 2 cases of array may be accessed out-of-bounds: Labeled code is unreachable: MediaInfoLib/Source/MediaInfo/Multiple/File_Mxf.cpp Line 14501 in 04880aa
Function uses more than 16 KB of stack: Access violation and debug assertion failed + allocate memory error as in issues 2105 and 2108. 989 cases of uninitialized member variables remain which I do not feel like fixing 😅 Notable things flagged by level 3 warnings but not by Code analysis: 19x signed/unsigned mismatch |
Thank you, very appreciated.
Another topic, when it is initialized but not in the constructor, only when I need, I need to refactor for having the constructor called when I need the value (and I initialize here) and not in advance, long term...
Definitely stuff I should care about, even if it is not impacting in practice it denotes a bad design somewhere in theory, I need to check that... when I have some free time :(. |
cjee21
changed the title
|
Cppcheck found more issues.... Most are similar to remaining ones from VS Code analysis as well as style-related that may or may not cause issues but some more significant ones are as below. Not sure if some may be false positives. Out-of-bounds, portability and performance
|
- Remove unused variables - Remove conditions that are always true - Variable `Parsers` can be declared as reference to const
cjee21
changed the title
|
Dead code block detected: MediaInfoLib/Source/MediaInfo/Video/File_Hevc.cpp Lines 4143 to 4154 in 04880aa
|
|
FYI some |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
MediaInfo_InformandFile_Mpeg4_Elementsflagged by Visual Studio Code analysis: