c89 compliance: move declaration outside for loop

[codebot]

to resolve discussion in #7

[mikaelarguedas]

Lgtm

[codebot]

linux CI: http://ci.ros2.org/job/ci_linux/2375/
osx CI: http://ci.ros2.org/job/ci_osx/1832/
windows CI: http://ci.ros2.org/job/ci_windows/2495/

builds were fine. there are a handful of test failures (1 on linux and OSX, 5 on windows), but they are unrelated to this change. Merging now.

[dirk-thomas]

I don't see the reason for this change. The previous code compiles perfectly fine on Windows. And the compiler as well as the warning should be selected for the non-test code anyway (not only for the tests). So I would propose to revert his change and address this comment instead.

[codebot]

Sorry I don't quite follow. Previously, the package didn't compile without tests enabled on gcc 4.9.2 on Jessie, as described in #7 . How else would we address the comment that you linked? Perhaps enabling C11 on Linux all the time, even when tests aren't enabled?

[dirk-thomas]

Yes, the standard version as well as the warning flags should be set outside of the test block (as it is done in all other ROS 2 packages).

[wjwwood]

We don't set C11 because windows doesn't fully support it. Neither rmw nor rcl set it. They set c++11 for their cpp test files only. Personally, I think this is the right change.

[dirk-thomas]

Windows supports the use case (to declare the variable within the loop) perfectly well. If in the future a new C11 specific is being used we and it turns out it doesn't work on Windows that simply can't be used. I don't see a reason why we should not utilize the features which are available all targeted platforms?

[wjwwood]

The issue is not whether or not windows support is this. It is Linux, and I don't think we should turn on c11 on Linux just for this feature because that risks us using a feature in c11 that windows doesn't support. Is it possible you could just set c99?

I don't see the issue, what Morgan proposed here works everywhere.

[dirk-thomas]

I am not proposing anything new here. Please see the current CMake file: it already uses C11. The flags just need to be set globally instead of in the test block. Simply because enabling / disabling tests should neither affect the C standard we use to build the code nor affect the warning level. I created PR #9 for this.

[mikaelarguedas]

Agreed that the C flags (if set) should be set everywhere and not only when building tests.
Regarding the variable declaration within loop, glad to see that windows supports it now, it was definitely not the case before. Which is why all of the C code doesn't use it. Happy to start using it if it's now supported everywhere.
Regarding using the flag itself I'm happy to use it if the features we use from it are supported by MSVC

[Karsten1987]

Sorry for the trouble here. That was definitely a mistake from my side. I should have set the C flags globally and only C++ in the tests. Must have been wrongly copy&pasted.

Can we officially support C99 on all platforms? I think MSVC doesn't support all of it but has adopted a few things: https://msdn.microsoft.com/en-us/library/hh409293.aspx

99 Conformance Visual Studio 2015 fully implements the C99 Standard Library, with the exception of any library features that depend on compiler features not yet supported by the Visual C++ compiler (for example, <tgmath.h> is not implemented).

Can we instead of setting c11 set c99 ?

[dirk-thomas]

Can we instead of setting c11 set c99?

Why should be restrict the features we can use as long as they are available all targeted platforms?

[wjwwood]

Why should be restrict the features we can use as long as they are available all targeted platforms?

I don't want to be cross-checking every feature of C11 with whether or not Windows supports it. Both rmw and rcl avoid setting C11 on purpose, I would have advised this package do the same had I noticed that it was using C11. The difference in C99 and C11 on Windows is that C99 is partially supported, but C11 is completely unsupported and they have hinted that they will never update their compiler to work with newer C standards.

[Karsten1987]

I am not saying we should restrict ourselves, but we could have a statement for a minimum requirement, such that we require a C99 compatible compiler.

Or do we have any use-case which requires C89? Any embedded processor etc?