False positive duplication detection in large switch yards

Report a False-positive / False-negative
, ,
#1

Template for a good false-positive report, formatted with Markdown:

  • versions used (SonarQube, Scanner, language analyzer)
    Version 8.9.2 (build 46101)
  • minimal code sample to reproduce (with analysis parameter, and potential instructions to compile).
    Currently our code base has a threshold for allowed duplication. Assume any threshold, say 1%. If you happen to have a fall through switch yard in different functions that list the same cases in the same order you get a false positive on duplication. It is currently a warning in gcc to not explicitly list all enum cases in a switch yard and if you treat warnings as errors you have to. So in order to successfully build, you will have to have all the cases listed. A workaround can be to be creative with the ordering of the cases in every case you need to avoid the false positive but that is not a proper solution. Ideally duplicate detection should be allowing situations where if all the detected lines start with “case” then its not considered.

Example the following code will be flagged with duplicate lines (ln22-29 & ln48-55)

If you do not handle all cases in the yard, most compilers will throw warnings about not treating all cases of the enum [-Wswitch]

#include <iostream>
#include <string>

enum class foods: int
{ 
     cake,
     cheesecake,
     icecream,
     scrambledEggs,
     eggsBenedict,
     applePie,
     peachPie,
     blueberryPie,
     steak,
     cheeseDip,
     hotDog
}; 

bool hasMeat(foods f){
    bool result{false};
    switch(f){
        case foods::cake:
        case foods::cheesecake:
        case foods::icecream:
        case foods::scrambledEggs:
        case foods::eggsBenedict:
        case foods::applePie:
        case foods::peachPie:
        case foods::blueberryPie:
        {
            result = false;
            break;
        }
        case foods::steak:
        case foods::cheeseDip:
        case foods::hotDog:
        {
            result = true;
            break;
        }
    }
    return result;
}

bool hasSugar(foods f){
    bool result{false};
    switch(f){
        case foods::cake:
        case foods::cheesecake:
        case foods::icecream:
        case foods::scrambledEggs:
        case foods::eggsBenedict:
        case foods::applePie:
        case foods::peachPie:
        case foods::blueberryPie:
        {
            result = true;
            break;
        }
        case foods::steak:
        case foods::cheeseDip:
        case foods::hotDog:
        {
            result = false;
            break;
        }
    }
    return result;
}

int main(){

    bool isSugarFree{!hasSugar(foods::cheesecake)};
    std::cout << std::boolalpha;
    std::cout << isSugarFree << '\n';

    bool isVegetarian{!hasMeat(foods::steak)};
    std::cout << isVegetarian << '\n';

    
    return 0;
}

Wrap code around triple quote ``` for proper formatting

#3

Hello @Evangelos_Denaxas and welcome to this forum,

I’m not really sure how to move forward with this situation. To me, this does look like duplicated code. It may be a case where duplication is justified (this happens), but this looks like duplication nevertheless.

There can be other ways to define the values associated with some enum members, that would not trigger the duplication rule, but they usually come with their own complexity, and I would not use them unless this patterns is common in your codebase (and the cost of getting used to this pattern is compensated by the number of time it is used).

For instance, you could ask the preprocessor to do the duplication for you:

  • In food.inc
// No header guard
// Macro FOOD(name, hasMeat, hasSugar)
FOOD(cake, false, false)
FOOD(steak, true, false)
// ...
  • In the rest of the code
enum class foods : int {
#define FOOD(name, sugar, meat) name,
#include "food.inc"
#undef food
};

bool hasMeat(foods f){
  switch(f) {
#define FOOD(name, sugar, meat) \
  case foods::name: return meat;
#include "food.inc"
#undef FOOD
};

In your case, I would probably just accept the duplication, but maybe reduce its size by putting the common case in a default:

bool hasMeat(foods f){
    switch(f){
        case foods::steak:
        case foods::cheeseDip:
        case foods::hotDog:
            return true;
        default:
            return false;
    }
}