[clang][analyzer] Allowing DeadStore to Ignore Certain Expressions

[bc-lee]

Motivation

Sometimes, it is necessary to keep an expression in the codebase for debugging purposes without using it in the release build. Common practices include using assert, NSAssert (from Cocoa), or DCHECK (from Various Google Projects). However, clang-tidy's deadcode.DeadStores check flags these expressions as dead stores, even though they serve a purpose in the debug build.

Example

CVPixelBufferRef pxbuffer = NULL;
CVReturn status = CVPixelBufferCreate(kCFAllocatorDefault, _width, _width, _pixelFormatType,
                                      (__bridge CFDictionaryRef)options, &pxbuffer);
NSAssert(status == kCVReturnSuccess, @"Failed to create CVPixelBuffer");
return pxbuffer;

In the above example, status is used solely for the NSAssert statement. In the release build, the NSAssert statement is removed, but clang-tidy still flags status as a dead store.

Proposed Solution

Introduce a deadcode.DeadStores check option named IgnoreExpressions. This option will accept a comma-separated list of expressions to ignore from the dead store check. If an expression matches any entry in the list, it will be ignored by the dead store check.

This proposal aims to improve the flexibility of clang-tidy by allowing developers to maintain necessary debug code without affecting the release build's static analysis checks.

[llvmbot]

@llvm/issue-subscribers-clang-static-analyzer

Author: Byoungchan Lee (bc-lee)

# Motivation Sometimes, it is necessary to keep an expression in the codebase for debugging purposes without using it in the release build. Common practices include using `assert`, `NSAssert` (from Cocoa), or `DCHECK` (from Various Google Projects). However, `clang-tidy`'s `deadcode.DeadStores` check flags these expressions as dead stores, even though they serve a purpose in the debug build. # Example ```objc CVPixelBufferRef pxbuffer = NULL; CVReturn status = CVPixelBufferCreate(kCFAllocatorDefault, _width, _width, _pixelFormatType, (__bridge CFDictionaryRef)options, &pxbuffer); NSAssert(status == kCVReturnSuccess, @"Failed to create CVPixelBuffer"); return pxbuffer; ``` In the above example, `status` is used solely for the `NSAssert` statement. In the release build, the `NSAssert` statement is removed, but `clang-tidy` still flags `status` as a dead store. # Proposed Solution Introduce a `deadcode.DeadStores` check option named `IgnoreExpressions`. This option will accept a comma-separated list of expressions to ignore from the dead store check. If an expression matches any entry in the list, it will be ignored by the dead store check. This proposal aims to improve the flexibility of `clang-tidy` by allowing developers to maintain necessary debug code without affecting the release build's static analysis checks.

[steakhal]

I can agree with the motivation. I can suggest two ways for fixing this:

1) Analyze your code in debug configuration, so asserts are present. This requires no analyzer changes, and readily available for the users. 2) Modify the predefines buffer in the preprocessor for the analysis by adding the #define DEBUG and #undef NDEBUG macros. This is a much more intrusive way achieving the same thing. This would transparently switch to debug mode. This might break assumptions in the code, leading to parsing errors. Consequently, this would need an escape hatch for disabling this behavior.

I feel, the 2nd option is more direct as it lessens the burden of the user and provides a better default. But this would need consensus before one could implement it.

---

w.r.t. your proposed solution: It's not clear to me from the proposal how it would match the ignored expression. This would only affect this single checker - which is a good and bad depending on how you look at it. Personally, I'd prefer a generic solution, but I'm considering checker-specific solutions as well.