search

biicode / client

Biicode client
http://www.biicode.com
MIT License
35 stars 15 forks source link

Show warnings only for open blocks #18

Open smessmer opened 9 years ago

smessmer commented 9 years ago

(see http://forum.biicode.com/t/show-warnings-only-for-open-blocks/334 )

I'd like to use warning flags like "-Wall -Wextra -Weffc++" on my code, but using

TARGET_COMPILE_OPTIONS(${BII_BLOCK_TARGET} INTERFACE -Wall -Wextra -Weffc++)

shows a lot of warnings for the blocks I'm including (e.g. boost).

Reasoning I generally assume the libraries I use are working, but might not be written using good C++ style. Some libraries might be written well, but still throw warnings on -Weffc++, because they use certain constructs for performance (e.g. boost often doesn't have virtual destructors on inherited classes).

As a developer using an arbitrary library, it is not necessarily my task to remove the warnings from their code (which might sometimes not be possible like in the boost example above), but I'd like to use the warnings mechanism to be notified of style issues in my own code. Warnings in code written by others just clutters the output and makes it harder for me to find the issues in my own code.

Many languages (e.g. Java, Scala, ...) only show you warnings in your own code and I think that this generally is the better approach. But because of the C++ header inclusion mechanism, compilers can't easily distinguish whether you're including a header you've written yourself or a header from a library. They actually can distinguish whether you're including a system header, and in this case, they also hide the warnings from there. But they can't easily distinguish 3rd party library headers from your own headers (the 3rd party library might just be in some subfolder of your project for example).

Different to plain compilers, biicode is in a position to easily decide whether a header is a 3rd party header or not (open blocks are own code, closed blocks 3rd party). So biicode might have the chance here to be better than previous systems.

Since this is a behavior C++ programmers might not be used to, an opt-in or opt-out could be offered. I'll definitely opt-in, because I think it is much better behavior.

Possible implementation I've read that including other libraries as system libraries makes gcc ignore warnings for these library headers. This could for example be done with

include_directories(SYSTEM "${LIB_DIR}/Include")

Is it possible to include closed blocks this way and therefore make gcc only show warnings for open blocks?

drodri commented 9 years ago

I am trying to do so, and added the SYSTEM option to all our TARGET_INCLUDE_DIRECTORIES, but doesnt seem to have the desired effect.

BTW, just in case you also want to try out things, there is always the option of running from source, it is not difficult to setup, and you will manage to run patches and fixes before the next release. But for the installed/compiled version, you can also hack the biicode.cmake template file. It is located in your installation => bii\biicode\client\dev\cpp\cmake folder. You can try to modify it, it will be used in your bii commands and projects. Just make sure to backup and restore it.

What I have tried is to add the SYSTEM option to all target_include_directories there, so it is actually being used for all includes, and still the warnings are propagated, will keep investigating.

drodri commented 9 years ago

I have figured out how to do it, but only for non absolute paths, i.e. not for #include "user/block/header.h" but for #include "header.h" and adding an [includes] with header.h: user/block.

Also I am configuring it as a opt-in functionality, as it breaks arduino builds. Are you using absolute or relative paths?

smessmer commented 9 years ago

For my blocks to work with simple project layout (on travis), I have to use absolute paths when including other blocks. Paths inside a block are relative, but this is my code, so I want to get the warnings.

How did you solve it? I'm wondering because it seems to be a compiler feature meant for system libraries, which usually are included using absolute paths as well.

Opt-in is fine for me.

smessmer commented 9 years ago

ah I misunderstood your last post - didn't see the thing about using the [includes] section before. Yes, this might be a workaround, although it would be better without of course.

What is the problem with absolute paths? Do relative paths in subfolders work ("folder/header.h")? Is it then possible to use a "relative" path "user/block/header.h" and map it in [includes] to "user/block/header.h"?

drodri commented 9 years ago

The current problem with absolute path, in order to achieve this SYSTEM functionality is that absolute paths are added to include directories in the project root CMakeLists.txt, and thus, there is no way to opt-in, opt-out this, as it is done before any other processsing.

Relative include paths, those defined by [includes] and [paths] are added to the include paths after, and thus can be user changed, or configured with opt-in opt out. Inside the same block, includes can (and should, if you want to open them in new layouts -L=simple), be relative, which can be achieved with the [paths] configuration.

Includes to other blocks can indeed be relative, using the [includes] section. Basically user/block/header.h is mapped as:

[includes] header.h: user/block

Wildcard patterns are allowed (gtest/*.h: google/gtest/include). This is very useful if you want to keep your code build-able without biicode, as the user/block will not be hardwired in code. I keep working trying to fix remaining tests for this issue. Even if I am not able to fix the absolute path case, I might try to release the other, so at least can be partly useful.

Diego Rodriguez-Losada CEO Biicode Innovation SL M: +34 656274654

2015-05-03 16:53 GMT+02:00 Sebastian Meßmer notifications@github.com:

ah I misunderstood your last post - didn't see the thing about using the [includes] section before. Yes, this might be a workaround, although it would be better without of course.

What is the problem with absolute paths? Do relative paths in subfolders work ("folder/header.h")? Is it then possible to use a "relative" path "user/block/header.h" and map it in [includes] to "user/block/header.h"?

— Reply to this email directly or view it on GitHub https://github.com/biicode/client/issues/18#issuecomment-98490190.

smessmer commented 9 years ago

Thanks for the detailed explanation, that makes sense.

If I understood you correctly, the problem with absolute paths is that biicode.conf isn't processed yet when adding them to include_directories. Maybe a solution could be to define opt-in/opt-out behavior not in biicode.conf, but somewhere else? Say settings.bii?

I'm not sure which is the better place anyhow. I'd like to have it in biicode.conf, so I don't have to reconfigure it per build client, but on the other hand this configuration might be a client decision that is/should be independent from the actual source code.

drodri commented 9 years ago

Hi Sebastian!

I have just pushed it to develop (you can use it from source, will be in next release): https://github.com/biicode/client/commit/92d9e4dae729dae6319462b7882bc437d092c776

To summarize:

$ bii configure -DBII_DEPS_SYSTEM=OFF

If you try it from source, or when next release is out, could you please tell me if it worked? Thanks!

smessmer commented 9 years ago

Just tested it from your develop branch. It works like a charm. This really improves my ability to keep my code clean, thanks you so much :)

One possibility for improvement: Also use this for boost libraries. When using the biicode/boost/setup repository with "-Wall -Wextra", I get a lot of warnings from boost (for example .biicode/boost/1.57.0/boost/spirit/home/classic/utility/functor_parser.hpp)

drodri commented 9 years ago

You are welcome :) that was a good suggestion, I also dislike so many warnings when building deps. The boost libraries are handled a bit differently, probably @Manu343726 who developed that block can have a look at it. Cheers!