False positive duplication detection in large switch yards

Rules and Languages Report 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;
    }
}
5

I would like to resurrect this thread. I agree that this should be considered a false positive. Large switch yards like these should not be considered duplicate code. Thereā€™s no real code in these. My college ran into this issue where thereā€™s duplication detection between two differerent functions that wrap switch statements where the functions have different return types and have different groups of enumerators that are handled uniquely.

Using default cases are a non starter when weā€™re intentially listing out all the values to get warned when new enumerators are added.

It would be extremely silly to switch up the order of the enumerators just to avoid the duplication detection, the ability to do that also sort of undermines the validity of considering these as duplicates.

8

We agree that this duplication detection is unfortunate. However, itā€™s not trivial to ignore them without creating other issues. I created a ticket to work on this, but cannot commit on any timeline.

1 Like