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

Enable minimal code style checks #116

Merged
merged 5 commits into from May 23, 2019

Conversation

@coatless
Copy link
Contributor

commented May 20, 2019

When building ensmallen on TravisCI, verify the code passes traditional ISO checks given by:

-Wall -Wpedantic -Wextra

If not, treat any warnings that arise as errors with -Werror. Show the errors clearly by colorizing the diagnostic output using -fdiagnostics-color.

These checks are generally used by CRAN:

https://cran.r-project.org/doc/manuals/r-release/R-exts.html#Writing-portable-packages

Enable minimal code style checks
When building ensmallen on TravisCI, verify the code passes traditional ISO checks given by:

```
-Wall -Wpedantic -Wextra
```

If not, treat any warnings that arise as errors with `-Werror`. Show the errors clearly by colorizing the diagnostic output using `-fdiagnostics-color`.

These checks are generally used by CRAN:

https://cran.r-project.org/doc/manuals/r-release/R-exts.html#Writing-portable-packages
@coatless

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

@zoq here is an implementation of the warning checks discussed in #115. These will only be enabled on TravisCI as I've set a SHOW_COMPILE_WARNINGS variable.

Note: For the initial go, I've upped the compilation standard to error on warnings (-Werror). This probably can be relaxed, but the -Wall -Wpedantic should be retained.

@coatless

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

The initial error was from Travis' compiler being less than gcc 4.9, which first included -fdiagnostics-color. To resolve this, I've removed it.

The next error occurs because #115 needs to be merged. (It's detecting those problems).

@rcurtin
Copy link
Member

left a comment

I like the idea, it would be nice to have a check like this. It could be in addition to any CI style checks / static code checks too.

.travis.yml Outdated
@@ -18,7 +18,7 @@ before_install:
- cmake . && make && sudo make install && cd ..

install:
- mkdir build && cd build && cmake .. && make -j2
- mkdir build && cd build && cmake -DSHOW_COMPILE_WARNINGS=ON .. && make -j2

This comment has been minimized.

Copy link
@rcurtin

rcurtin May 21, 2019

Member

Hmm, could we simplify by just calling cmake -DCMAKE_CXX_FLAGS='-Wall -Wpedantic -Wextra -Werror' ..? Or is there a compelling reason to add the SHOW_COMPILE_WARNINGS option? (maybe it makes life easier downstream?)

This comment has been minimized.

Copy link
@coatless

coatless May 21, 2019

Author Contributor

Definitely. Though, how would you feel if I just mirrored MLPACK's setup?

https://github.com/mlpack/mlpack/blob/3389a40ecd686a7e1e01a63b475b042e804440ff/CMakeLists.txt#L65-L78

This comment has been minimized.

Copy link
@rcurtin

rcurtin May 21, 2019

Member

That's totally fine with me. We can use the warning flags on every build if you like (except -Werror I would say, I would agree with Conrad that that can be really annoying). But perhaps it is good to enable -Werror just for the Travis build (via CMAKE_CXX_FLAGS) so we avoid committing any code that causes warnings.

# Moreover, treat any compilation warning as an error.
if ( SHOW_COMPILE_WARNINGS )
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wall -Wpedantic -Wextra -Werror")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wpedantic -Wextra -Werror")

This comment has been minimized.

Copy link
@conradsnicta

conradsnicta May 21, 2019

Contributor

Be careful with using -Wextra. It can produce overly pedantic warnings. Coupling this with -Werror could get annoying real fast. Perhaps a selected subset of the warnings by enabled by -Wextra can be useful. Examples: -Wunused-parameter and -Wunused-but-set-parameter. I'd recommend to also enable -Wshadow

This comment has been minimized.

Copy link
@coatless

coatless May 21, 2019

Author Contributor

@conradsnicta will do. Thanks for the suggestion!

coatless added some commits May 21, 2019

Partially align CMAKE code with MLPACK
- Removed the "SHOW_COMPILE_OPTIONS"
- Added chunk to detect compiler and set appropriate values
- Dropped `-Wextra` in favor of specific diagnostics
@zoq zoq referenced this pull request May 21, 2019
@coatless

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

After merging in PR #115 that corrected the compile warnings, this PR builds nicely.

The flags set now are:

-Wall -Wpedantic -Wunused-parameter

MLPACK uses -Wall -Wextra.

Adding in -Wshadow would result in quite a few changes needed. So, I've opted to leave it off the list for now.

@zoq

zoq approved these changes May 22, 2019

Copy link
Member

left a comment

Thanks, looks good to me.

@rcurtin
Copy link
Member

left a comment

👍 thanks!

@rcurtin rcurtin merged commit ff44866 into mlpack:master May 23, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zoq zoq referenced this pull request May 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.