platform-ststm32 icon indicating copy to clipboard operation
platform-ststm32 copied to clipboard

Treat CMSIS and HAL includes as system ones

Open yaqwsx opened this issue 5 years ago • 12 comments

The keyword register is deprecated in C++17. CMSIS and HAL libraries use it a lot in their headers. Therefore, when compiling a project with C++17 flags, the user gets a lot of irrelevant warnings during compilation like:

warning: ISO C++1z does not allow 'register' storage class specifier [-Wregister]

It makes the compiler output practically unusable.

I think the CMSIS and HAL includes should be treated as a system includes - one of the effects is that compiler does not show warnings from these headers as the user cannot change anything about it.

This PR changes that. There is no direct way of specifying it in SCons (according to StackOverflow), therefore it has to be done directly via CPPFLAGS. It should cause no harm, as GCC is always used as the compiler in this case.

yaqwsx avatar May 21 '20 14:05 yaqwsx

Hi @yaqwsx ! Many thanks for the PR. Personally, I don't think it's a good idea to hide such warnings. The issue should be properly fixed in the source code of the CMSIS/HAL.

valeros avatar May 21 '20 15:05 valeros

It is not a warning in user code and it is part of code, which is out of your control. You cannot fix it (unless you want to maintain your own version of CMSIS/HAL). This is the reason why the -isystem exists in the first place.

Also, in some sense, CMSIS and HAL is the system include for this platform.

Would you at least consider accepting a PR where usage of -isystem would be configurable via user ini file?

yaqwsx avatar May 21 '20 16:05 yaqwsx

Sorry for the late reply.

It is not a warning in user code and it is part of code, which is out of your control. You cannot fix it (unless you want to maintain your own version of CMSIS/HAL). This is the reason why the -isystem exists in the first place.

Also, in some sense, CMSIS and HAL is the system include for this platform.

I agree that it sounds reasonable, but by that logic, shouldn't we treat include paths for all frameworks (not only CMSIS/HAL) as -isystem paths since users have no control over that source files?

The main reason why I'm so cautious is that your changes could possible break our IDE feature as not all our templates support -isystem flag.

As a workaround, have you considered using an extra script that converts framework includes from -I to -isystem ? Something like:

Import("env", "projenv")

FRAMEWORK_DIR = env.PioPlatform().get_package_dir("framework-stm32cube")

plain_includes = []
system_include = []

for inc in env["CPPPATH"]:
    if inc.startswith(FRAMEWORK_DIR):
        system_include.append(inc)
    else:
        plain_includes.append(inc)

projenv.Replace(CPPPATH=plain_includes)
projenv.Append(CPPFLAGS=[("-isystem", inc) for inc in system_include])

valeros avatar May 22 '20 13:05 valeros

I agree that all "platform system" includes should be treated as system ones.

The script you provided is a workaround and it will work for me. However, I am worried it will get lost in this issue and the problem will remain for the other users of PIO on STM32 using modern C++.

yaqwsx avatar May 28 '20 06:05 yaqwsx

Now you can add build_flags =-Wno-register to your platformio.ini for mute this warnings.

MasterLufier avatar Jan 21 '21 18:01 MasterLufier

@MasterLufier: Well, I would like to specify this only for the external libraries that are not under my control - not for all sources. And that's why GCC offers "system includes" for the code that is out of your control. However, maintainers of the PIO do not share the same opinion as I do - that all frameworks should be treated as system includes as they are out of users' control.

yaqwsx avatar Jan 21 '21 18:01 yaqwsx

@yaqwsx I think it's doesn't matter, so you do not use register in new code. register keyword actually do nothing now and you can switch off this warning without any side-effects.

MasterLufier avatar Jan 21 '21 19:01 MasterLufier

Of course; in practice, this works great for the -Wregister warning. However, I compile my code with -Wall -Wextra and -Wpedantic, and that triggers a lot of warnings from HAL includes that are simply out of my control and I want to ignore them. But I do not want to ignore those warnings on my code.

yaqwsx avatar Jan 21 '21 19:01 yaqwsx

You can mix -Wno-register with -Wall -Wextra and -Wpedantic. It's not a problem. All other warnings will be showing.

MasterLufier avatar Jan 21 '21 19:01 MasterLufier

I am sorry if I expressed myself wrongly; I do not want to disable warnings based on "what is it" but "where does it come from". I do not want to see any warnings from HAL, LL, or CMSIS as it is a code out of my control and I cannot do anything about it. Having them in compilation output just makes it messy and you easily miss relevant warnings.

PS: This is getting a little bit flameworthy and not constructive; the maintainers have already expressed themself, I respect it, and also there is a proper workaround in the post from May 22 2020. All I wanted to point out is that your solution is not a solution for the original problem - just for a reader who googles up this PR.

PS: I probably missed mentioning the obvious; when you compile a code with -Wall -Wextra and -Wpedantic that includes HAL the compilation output is extremely noisy.

yaqwsx avatar Jan 21 '21 19:01 yaqwsx

@yaqwsx I apologize. I really misunderstood you. I thought the problem was only related to ' register` (and it's really thousands of messages in the debugger).

MasterLufier avatar Jan 21 '21 20:01 MasterLufier

FWIW, I noticed that the release notes for STM32CubeF4 v1.26.0 mentions:

Remove “register” keyword to be compliant with new C++ rules: The register storage class specifier was deprecated in C++11 and removed in C++17.

Source: https://htmlpreview.github.io/?https://github.com/STMicroelectronics/STM32CubeF4/blob/master/Release_Notes.html

There's an open issue to update the STM32CubeF4 framework from 1.25.2 to 1.26.x in https://github.com/platformio/platform-ststm32/issues/554.

Maybe that would help resolve the issue being discussed here?

noahwilliamsson avatar Aug 19 '21 10:08 noahwilliamsson