Skip to content

Conversation

@bobtista
Copy link

@bobtista bobtista commented Dec 6, 2025

Ran clang-tidy (bugprone-suspicious-string-compare)

@bobtista bobtista force-pushed the bobtista/refactor/clang-tidy-strcmp-compare branch from dcec451 to b15065d Compare December 6, 2025 18:57
@bobtista bobtista added Minor Severity: Minor < Major < Critical < Blocker Refactor Edits the code with insignificant behavior changes, is never user facing labels Dec 6, 2025
Copy link

@xezon xezon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am surprised that no file of tools was touched. Is this correct or did it not search in Tools?

@xezon xezon added this to the Code foundation build up milestone Dec 7, 2025
@bobtista
Copy link
Author

bobtista commented Dec 7, 2025

I am surprised that no file of tools was touched. Is this correct or did it not search in Tools?

This seems to be about MFC and clang-tidy not mixing well. 90+% of tools files are skipped because of this.

@Skyaero42
Copy link

Skyaero42 commented Dec 7, 2025

These are the ones it missed

PeerThread
2593: if (strcmp(key, "username") && strcmp(key, "b_flags"))

StdLocalFileSystem
244: (strcmp(filenameStr.c_str(), ".") != 0 && strcmp(filenameStr.c_str(), ".."))) {
271: (strcmp(filenameStr.c_str(), ".") != 0 && strcmp(filenameStr.c_str(), ".."))) {

Win32LocalFileSystem
142: (strcmp(findData.cFileName, ".") != 0 && strcmp(findData.cFileName, ".."))) {
168: (strcmp(findData.cFileName, ".") != 0 && strcmp(findData.cFileName, ".."))) {

Download.cpp
50: if ((strcmp(m_Server, server) != 0) || (strcmp(m_Login, username)))

W3DView.cpp
347: !strcmp(search_ref->File, ref->File) &&

Directory.cpp
100: if ( strcmp( item.cFileName, "." ) && strcmp( item.cFileName, ".." ) )

assetmgr.cpp
1350: if (!stricmp(name, node->Data()->Name)) {

texturethumbnail.cpp
40: WWASSERT(!stricmp(&name[len-4],".tga") || !stricmp(&name[len-4],".dds"));

There are also indirect issues where the result of str(i)cmp is returned e.g. ASciistring::compare.
For example controlbar.cpp line 766 this goes wrong. But I haven't looked at all of them yet.

@bobtista bobtista force-pushed the bobtista/refactor/clang-tidy-strcmp-compare branch from b15065d to 3577e04 Compare December 9, 2025 04:51
@bobtista
Copy link
Author

bobtista commented Dec 9, 2025

These are the ones it missed

PeerThread
2593: if (strcmp(key, "username") && strcmp(key, "b_flags"))

StdLocalFileSystem
244: (strcmp(filenameStr.c_str(), ".") != 0 && strcmp(filenameStr.c_str(), ".."))) {
271: (strcmp(filenameStr.c_str(), ".") != 0 && strcmp(filenameStr.c_str(), ".."))) {

Win32LocalFileSystem
142: (strcmp(findData.cFileName, ".") != 0 && strcmp(findData.cFileName, ".."))) {
168: (strcmp(findData.cFileName, ".") != 0 && strcmp(findData.cFileName, ".."))) {

Download.cpp
50: if ((strcmp(m_Server, server) != 0) || (strcmp(m_Login, username)))

W3DView.cpp
347: !strcmp(search_ref->File, ref->File) &&

Directory.cpp
100: if ( strcmp( item.cFileName, "." ) && strcmp( item.cFileName, ".." ) )

assetmgr.cpp
1350: if (!stricmp(name, node->Data()->Name)) {

texturethumbnail.cpp
40: WWASSERT(!stricmp(&name[len-4],".tga") || !stricmp(&name[len-4],".dds"));

There are also indirect issues where the result of str(i)cmp is returned e.g. ASciistring::compare. For example controlbar.cpp line 766 this goes wrong. But I haven't looked at all of them yet.

the ones that are not in Tools should be fixed now

Copy link

@xezon xezon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change complete or still missing edits?

@bobtista bobtista force-pushed the bobtista/refactor/clang-tidy-strcmp-compare branch from 8a1192f to 600b528 Compare December 10, 2025 16:43
@bobtista
Copy link
Author

Is this change complete or still missing edits?

I ran it again, should be complete

@bobtista bobtista force-pushed the bobtista/refactor/clang-tidy-strcmp-compare branch from 600b528 to a15d54b Compare December 13, 2025 23:20
@bobtista
Copy link
Author

I found and fixed the rest that I could find with manual search (in the tools)

Copy link

@xezon xezon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks all correct to me. I did not check if this is complete.

This change must not be merged before #1989, because texturethumbnail.cpp will conflict.

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

Labels

Minor Severity: Minor < Major < Critical < Blocker Refactor Edits the code with insignificant behavior changes, is never user facing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants