Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement user-selectable compiler warning switches #60

Closed
skliper opened this issue Sep 30, 2019 · 9 comments
Closed

Implement user-selectable compiler warning switches #60

skliper opened this issue Sep 30, 2019 · 9 comments
Milestone

Comments

@skliper
Copy link
Contributor

skliper commented Sep 30, 2019

It has become apparent that a "one-size-fits-all" approach to compiler warnings may not be sufficient going forward.

In general, most developers should be using "-Werror" in addition to "-Wall", "-pendantic", etc to catch any coding issues at first sight.

However, the specific set of warnings for any given piece of code is dependent on the specific compiler version, target architectures, and optimization level in use at the time of build. This can mean code that builds without warnings on one build can fail miserably on a different build.

This could be an major issue for a novice who clones the latest code from the community repository, and builds it with the latest version of GCC (i.e. one that we may not have tested yet) and finds a new warning that causes the entire build to fail unexpectedly.

As a compromise, the following is proposed:

  • Always use "-Wall" switch to enable the most reasonable warnings (incidentally, this is not all warnings, it leaves out the ones most likely to generate false positives).
  • Do not put "-Werror" in the official build scripts, so if compiling with a new GCC version or a different target architecture than the what has been officially tested, the build will not fail if a new warning is triggered.
  • Also leave out "-pedantic" from the official build as this, by definition, tends to warn on constructs that are generally OK in practice but violate some (possibly esoteric) aspect of the C standard. The thought behind this is that plenty of old existing code out there may work fine but might not compile cleanly using "-pedantic", so we should not force this switch upon users by default.
  • Add a mechanism by which developers can easily add extra CFLAGS to a build, without modifying a Makefile (or CMakeLists) file. This way, any users that want to may enable "-Werror" or "-pedantic" on their own builds, all the time, without having to maintain a private branch of the build script.

The automatic builds done by Bamboo, these will be built without "-Werror" but the build log will be checked for warnings, and the presence of any new warnings will be logged as a unit test error so they can be fixed before moving the code forward.

@skliper skliper added this to the osal-4.2 milestone Sep 30, 2019
@skliper skliper self-assigned this Sep 30, 2019
@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Imported from trac issue 37. Created by jphickey on 2015-04-17T14:18:14, last modified: 2015-11-20T16:22:16

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jphickey on 2015-04-17 14:52:46:

Commit [changeset:f46224c] has been pushed for this

The extra flags are specified through a variable called "OSAL_USER_C_FLAGS". The value of this variable is set in one of two ways:

  • Via an environment variable, e.g. {{{export OSAL_USER_C_FLAGS=-Werror}}}
  • Via a "-D" option to the cmake commend, i.e. {{{-DOSAL_USER_C_FLAGS=-Werror}}}

In all cases, a CMake cache variable is utilized so that value will be retained from build to build, even if the original environment variable changes. This is important for building from an IDE that may have a different environment than the command shell.

Also, the {{{-Wall}}} build option is added to all builds.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by glimes on 2015-05-18 20:04:56:

Accept.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by sstrege on 2015-05-19 10:38:17:

What is the mechanism by which developers can easily add extra CFLAGS to a build using the old build system?

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by sstrege on 2015-06-29 16:53:27:

The answer to my question above is provided in the comments (through an environment variable).

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by sstrege on 2015-06-29 16:53:39:

Accept

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by glimes on 2015-06-30 11:02:42:

Recommend accept.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by acudmore on 2015-06-30 12:40:43:

Recommend accept.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by glimes on 2015-06-30 14:37:42:

CCB approved.

@skliper skliper closed this as completed Sep 30, 2019
@skliper skliper removed their assignment Sep 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant