llvm / llvm/llvm-project

[clang-tidy] Conflicts between cppcoreguidelines-init-variables and clang-analyzer-deadcode.DeadStores

Open
#208,187 4 comments 0 reactions 0 assignees View on GitHub
clang:static analyzer
Dominant language
LLVM
Stars
40.5k
Forks
18.7k
PR merge metrics
PR metrics pending

Description

It is difficult to make both `cppcoreguidelines-init-variables` and `clang-analyzer-deadcode.DeadStores` happy, at least without adding additional redirections.

Just as an example

```c++
enum class e {e1=1,e2,e3};

void foo(int i){

e value;
switch(i){
case 1: value=e::e1;
case 2: value=e::e2;
case 3: value=e::e3;
default: return; // or default: value=e1;
}

// use value from here on
}
```

Since `e value;` does not initialize the value, `cppcoreguidelines-init-variables` emits a diagnostic.

But if the code is changed to `e value = {};`, then `clang-analyzer-deadcode.DeadStores` complains, because `value` is always overwritten.

If the switch looks like

```c++
switch(i){
case 1: value=e::e1;
case 2: value=e::e2;
case 3: value=e::e3;
default: value=e::e1;
}
```

then the code can be rewritten as

```c++
auto value = [&]{switch(i){
case 1: return e::e1;
case 2: return e::e2;
case 3: return e::e3;
default: return e::e1;
}}();
```

but with the `return` statement, more boilerplate is necessary, which makes the code harder to follow.

I think there is an advantage of having the following pattern accepted by `cppcoreguidelines-init-variables` (with an option eventually), instead of locally silencing the warning

```c++
e value;
switch(i){
case 1: value = e::e1;
case 2: value = e::e2;
case 3: value = e::e3;
default: return;
}
// use value from here on
```

If a new `case` is added, which does not initialize `value`, then `cppcoreguidelines-init-variables` would diagnose it.
If the warning was silenced locally, then there would be no warning for the new `case` statement.

Changing the code to

```c++
e value = {};
switch(i){
case 1: value = e::e1;
case 2: value = e::e2;
case 3: value = e::e3;
default: return;
}
// use value from here on
```

has the disadvantage that adding a new `case` that does not initialize `value` properly will not be diagnosed.

Contributor guide

Open the contributing guide

Research direction

Start by reproducing the interaction between cppcoreguidelines-init-variables and clang-analyzer-deadcode.DeadStores with the switch examples in the issue. Read the implementation and tests for both named clang-tidy checks, then determine how the proposed pattern and any option should be specified. Done means an agreed behavior is implemented and covered by tests without weakening detection of newly added uninitialized cases.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
compilers, devtools
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.