ravioli icon indicating copy to clipboard operation
ravioli copied to clipboard

`break` in switch/cases identified as global variables

Open C47D opened this issue 6 years ago • 2 comments

Hi,

ravioli (0.3.0) is identifing the breaks inside a switch/case on my update_config function as global variables, is this expected?

static void update_config(void)
{
	uint8_t config = (HAL_GPIO_ReadPin(SW1_GPIO_Port, SW1_Pin) << 1) | HAL_GPIO_ReadPin(SW2_GPIO_Port, SW2_Pin);

	switch (config) {
	case 0x00:
		new_value = VREF_LVL1;
		break;
	case 0x01:
		new_value = VREF_LVL2;
		break;
	case 0x02:
		new_value = VREF_LVL3;
		break;
	case 0x03:
		new_value = VREF_LVL4;
		break;
	default:
		break;
	}

	HAL_TIM_PWM_Stop(&htim2, TIM_CHANNEL_2);
	__HAL_TIM_SET_COMPARE(&htim2, TIM_CHANNEL_2, new_value);
	HAL_TIM_PWM_Start(&htim2, TIM_CHANNEL_2);
}

I guess it can be changed (below), but it becomes less readable:

static void update_config(void)
{
        uint8_t new_value[] = {VREF_LVL1, VREF_LVL2, VREF_LVL3, VREF_LVL4};

	uint8_t config = (HAL_GPIO_ReadPin(SW1_GPIO_Port, SW1_Pin) << 1) | HAL_GPIO_ReadPin(SW2_GPIO_Port, SW2_Pin);

        /* error handling */

	HAL_TIM_PWM_Stop(&htim2, TIM_CHANNEL_2);
	__HAL_TIM_SET_COMPARE(&htim2, TIM_CHANNEL_2, new_value[config]);
	HAL_TIM_PWM_Start(&htim2, TIM_CHANNEL_2);
}

C47D avatar Oct 24 '19 20:10 C47D

I have the exact same issue. I assume it is due to the way the scanning works.

My example is:

  switch (_pwm.state)
  {
    case PWM_INIT_ST:
      if (sanity_checks_OK())
      {
        do_stuff();
      }
      break;

Globals

The tool outputs:

src\waveform\pwm.c:0 break
src\waveform\pwm.c:189 break
src\waveform\pwm.c:189 break
src\waveform\pwm.c:189 break
src\waveform\pwm.c:0 break
src\waveform\pwm.c:189 break

Where line 0 has nothing like that on it. And line 189 is the link after the } of the if statement.

Note that I think it is due to the brace matching that is done. It sees the end brace }, and thinks the next variable is a global (which it sees as a break statement).

Shouldn't the tool do brace matching?

The code:

code = re.sub(r'{[^}]*}', '{}', code, flags=re.DOTALL)

Looks to not handle this correctly, it is the same with if statements too, but these don't seem to trigger the global count. I think what the code should do is find a brace and then skip everything until it has its matching end brace, not until it finds the first end brace.

The solution might be something like this: https://stackoverflow.com/questions/14596884/remove-text-between-and-in-python

bdun avatar Aug 02 '20 11:08 bdun

Hello! No this is not expected.

It turns out the the way ravioli counts globals is quite naive -- and likely to be quite buggy -- and also isn't correct for the way KSF is defined.

What it needs to count are global usages not merely global definitions. This is a bit more complicated problem, but the right thing to do.

I'm currently working on improvements to global counting. When this is done, this should potentially fix a variety of issues.

mchernosky avatar Aug 27 '21 22:08 mchernosky