Skip to content

Run scanbuild with asserts enabled, as suggested by its manual #47

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Voker57
Copy link
Contributor

@Voker57 Voker57 commented Feb 15, 2025

No description provided.

@afwbkbc
Copy link
Owner

afwbkbc commented Feb 15, 2025

I don't think it's good idea, it's more important to check Release because that's what most (or all) players will use.
In some cases something may work in debug because of ASSERT, while in Release it may produce silent bugs, this should be checked

@Voker57
Copy link
Contributor Author

Voker57 commented Feb 16, 2025

I don't think it's good idea, it's more important to check Release because that's what most (or all) players will use. In some cases something may work in debug because of ASSERT, while in Release it may produce silent bugs, this should be checked

can do both, but debug is more important for static analysis as it includes more code and enables assertions which help to analyze the code.

@afwbkbc
Copy link
Owner

afwbkbc commented Feb 16, 2025

'both' would make builds even slower than it is now.
Debug builds may include more code but that extra code won't be used by actual players, also it's a bit different from release so something may be broken in release but not debug (in this case analyzer won't find it if it will test debug). Debug builds can also have some ifdefs with quick hacks to test something that analyzer will go crazy about (and I don't feel like polishing debug builds to keep analyzer happy 24/7)

@Voker57 Voker57 changed the title Run scanbuild in debug mode, as suggested by its manual Run scanbuild with asserts enabled, as suggested by its manual Feb 16, 2025
@Voker57
Copy link
Contributor Author

Voker57 commented Feb 16, 2025

Changed the PR to instead enable asserts for more accurate analysis.

@afwbkbc
Copy link
Owner

afwbkbc commented Mar 6, 2025

Have you checked that force-analyze-debug-code actually does anything?
In GLSMAC asserts are not normal C++ asserts, but

#if defined( DEBUG ) || defined( FASTDEBUG )

#define ASSERT( _condition, _text ) \
    if ( !( _condition ) ) { \
        Log( (std::string) "FATAL: " + _text ); \
        THROW( _text ); \
    }

// for rare cases when Log() is not available
#define ASSERT_NOLOG( _condition, _text ) \
    if ( !( _condition ) ) { \
        THROW( _text ); \
    }

#else
#define ASSERT( _condition, _text )
#define ASSERT_NOLOG( _condition, _text )
#endif

meaning they are removed in Release during preprocessing.

I checked and haven't found any good justification of scanning in debug mode instead of release.
There is:

2.2.2.8. Always Analyze a Project in its “Debug” Configuration

Most projects can be built in a “debug” mode that enables assertions. Assertions are picked up by the static analyzer to prune infeasible paths, which in some cases can greatly reduce the number of false positives (bogus error reports) emitted by the tool.

which gives no reasons and then just explains how to deal with problems of scanning debug builds (false positives).

@Voker57
Copy link
Contributor Author

Voker57 commented Mar 12, 2025

Perhaps use normal assertions instead of your bikeshed stuff then? No idea if it does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants