MarlinFirmware / Marlin

Marlin is an optimized firmware for RepRap 3D printers based on the Arduino platform. Many commercial 3D printers come with Marlin installed. Check with your vendor if you need source code for your specific machine.
https://marlinfw.org
GNU General Public License v3.0
16.28k stars 19.24k forks source link

[BUG] Warning: comparison of integer expressions of different signedness: 'const int' and 'const unsigned int' #27271

Closed classicrocker883 closed 3 months ago

classicrocker883 commented 3 months ago

Did you test the latest bugfix-2.1.x code?

Yes, and the problem still exists.

Bug Description

I noticed this recently: Link to where the warning is present under "> Run STM32F103RE_creality Tests"

CrealityUI configs (to test on your own)

In file included from Marlin\src\module\../inc/MarlinConfigPre.h:37,
                 from Marlin\src\module\../inc/MarlinConfig.h:28,
                 from Marlin\src\module\stepper.h:44,
                 from Marlin\src\module\stepper.cpp:86:
Marlin\src\module\../inc/../core/macros.h: In instantiation of 'constexpr decltype ((lhs + rhs)) _MAX(L, R) [with L = int; R = unsigned int; decltype ((lhs + rhs)) = unsigned int]':
Marlin\src\module\stepper.h:302:13:   required from here
Marlin\src\module\../inc/../core/macros.h:395:20: warning: comparison of integer expressions of different signedness: 'const int' and 'const unsigned int' [-Wsign-compare]
  395 |         return lhs > rhs ? lhs : rhs;


The code in question: Marlin\src\module\stepper.h:302 - _MAX((STEPPER_TIMER_RATE) / HAL_TIMER_TYPE_MAX, 1U)

So, the warning goes away when removing the U in 1U so it is just 1

Also, another workaround is under macros.h:

  #ifndef _MINMAX_H_
  #define _MINMAX_H_

  #include <type_traits>

    extern "C++" {
...

      template <class L, class R> static constexpr auto _MAX(const L lhs, const R rhs) -> typename std::common_type<L, R>::type {
        using common_type = typename std::common_type<L, R>::type;
        return (static_cast<common_type>(lhs) > static_cast<common_type>(rhs)) ? static_cast<common_type>(lhs) : static_cast<common_type>(rhs);
      }

however, this is probably related to a recent commit, so that should be looked into.

Bug Timeline

Within the past couple days. You can look at a pull request check (STM32F103RE_creality) from 4 days ago (since 7/15) and the warning is not there

Expected behavior

Should be no warning

Actual behavior

Warning when using Creality configs/CI Build Tests (Actions)

Steps to Reproduce

  1. Load up CrealityUI configs
  2. STM32F103RE_creality in platformio.ini
  3. Build - observe terminal warnings

Option B:

  1. View a recent pull request (within a couple days of 7/15)
  2. Under checks look for "CI - Build Tests / Build Test (STM32F103RE_creality) (pull_request)"
  3. Click Details
  4. Under "> Run STM32F103RE_creality Tests" you should see the multiple (same) warnings

Version of Marlin Firmware

bugfix-2.1.x

Printer model

Creality Ender-3

Electronics

BOARD_CREALITY_V4

LCD/Controller

DWIN_CREALITY_LCD

Other add-ons

No response

Bed Leveling

None

Your Slicer

None

Host Software

None

Don't forget to include

Additional information & file uploads

Configs used

ellensp commented 3 months ago

https://github.com/MarlinFirmware/Marlin/pull/26881 from 2 days ago. This was added to stepper.h _MAX((STEPPER_TIMER_RATE) / HAL_TIMER_TYPE_MAX, 1U) // 32-bit shouldn't go below 1

This triggers the warning.

ellensp commented 3 months ago

I purpose

diff --git a/Marlin/src/module/stepper.h b/Marlin/src/module/stepper.h
index 2a171bebd0..c24953c24e 100644
--- a/Marlin/src/module/stepper.h
+++ b/Marlin/src/module/stepper.h
@@ -299,7 +299,7 @@ class Stepper {
     // and avoid the most unreasonably slow step rates.
     static constexpr uint32_t minimal_step_rate = (
       #ifdef CPU_32_BIT
-        _MAX((STEPPER_TIMER_RATE) / HAL_TIMER_TYPE_MAX, 1U) // 32-bit shouldn't go below 1
+        _MAX((uint32_t(STEPPER_TIMER_RATE) / HAL_TIMER_TYPE_MAX), 1U) // 32-bit shouldn't go below 1
       #else
         (F_CPU) / 500000U   // AVR shouldn't go below 32 (16MHz) or 40 (20MHz)
       #endif

as STEPPER_TIMER_RATE/HAL_TIMER_TYPE_MAX can never be negative

classicrocker883 commented 3 months ago

why not just make it _MAX((STEPPER_TIMER_RATE) / HAL_TIMER_TYPE_MAX, 1)?

the end result would be no different.

ellensp commented 3 months ago

Int comparisons are faster than signed and can use less code, does it make a lot of different, no, just marginally better

github-actions[bot] commented 1 month ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.