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

Misc cxx fixes #5642

Merged
merged 2 commits into from
Jan 11, 2018
Merged

Conversation

SebastianBoe
Copy link
Collaborator

Port misc. constructs that caused compilation errors when included into CXX code. See commit messages for details.

I was under the impression that we compiled our C and CXX code in such a way that when it built with GCC, it was guaranteed to build with CXX as well. But these examples seem to disprove this.

Long term, for each of these constructs it should be investigated how to either:

Trigger a compilation error when compiling with GCC.
Make the CXX accept the construct as GCC does.

These cases were found by building the cpp sample with all bluetooth headers included.

Compiling this declaration with a CXX compiler triggers the compiler
error:

/home/sebo/zephyr/include/bluetooth/gatt.h:898:10: error: ‘struct
bt_gatt_read_params::<anonymous union>::__single’ invalid; an
anonymous union can only have non-static data members [-fpermissive]

Reading up on the standard, I was unable to find any mention of this
being valid C or CXX code (But reading the standard is not
straightforward). And I was unable to find any mechanism to make the
CXX compiler accept it (e.g. Changing the -std, or adding this as a
language extension e.g. -fms-extensions).

So we rewrite it to not declare the struct with the tag
"__single". There does not seem to be any reason for it to be declared
like this.

Signed-off-by: Sebastian Bøe <[email protected]>
The function returns an enum, not a u8_t, so we should cast
appropriately to avoid an implicit conversion.

This construct was triggering this compilation error when compiling
with CXX:

/home/sebo/zephyr/include/bluetooth/buf.h:85:9: error: invalid
  conversion from ‘u8_t {aka unsigned char}’ to ‘bt_buf_type’
  [-fpermissive] return *(u8_t *)net_buf_user_data(buf);

Signed-off-by: Sebastian Bøe <[email protected]>
@galak
Copy link
Collaborator

galak commented Jan 11, 2018

Do we need to improve the cpp sample / test to catch such issues in the future?

@codecov-io
Copy link

Codecov Report

Merging #5642 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5642   +/-   ##
=======================================
  Coverage   53.56%   53.56%           
=======================================
  Files         428      428           
  Lines       41191    41191           
  Branches     7856     7856           
=======================================
  Hits        22062    22062           
  Misses      19018    19018           
  Partials      111      111

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 907735d...7110098. Read the comment docs.

@nashif
Copy link
Member

nashif commented Jan 11, 2018

Do we need to improve the cpp sample / test to catch such issues in the future?

we do need more samples with expanded coverage built with c++, right now we only have one sample.

@SebastianBoe
Copy link
Collaborator Author

Do we need to improve the cpp sample / test to catch such issues in the future?

Yes, not sure how though, perhaps we could have a sample that includes as many header files in zephyr/include as possible ...

@zigarrre
Copy link

zigarrre commented Jan 11, 2018

I would suggest not just expanding the samples but adding automated test cases to test as much of the API for C++ compatibility as possible.

Maybe adopt the existing C test cases so they are valid in both C and C++ and then just compile them once as C and once as C++ when executing the automated tests?

perhaps we could have a sample that includes as many header files in zephyr/include as possible ...

Including isn't enough though. A lot of errors are only triggered when calling functions, using macros or instantiating structs.

Also this PR barely scratches the surface of the problem. There are many more errors in various headers. Zephyr is essentially unusable with C++ at the moment.

@nashif
Copy link
Member

nashif commented Jan 11, 2018

Also this PR barely scratches the surface of the problem. There are many more errors in various headers. Zephyr is essentially unusable with C++ at the moment.

well, I would not call it unstable, it does not build... :)

Can you share what exactly you were building which discovered those issues?

@nashif nashif merged commit d3304dc into zephyrproject-rtos:master Jan 11, 2018
@zigarrre
Copy link

zigarrre commented Jan 11, 2018

This would be pretty much the minimum code to reproduce not only the problems fixed in this PR but even more.

#include <zephyr.h>

#include <bluetooth/bluetooth.h>
#include <bluetooth/hci.h>
#include <bluetooth/conn.h>
#include <bluetooth/uuid.h>
#include <bluetooth/gatt.h>

static const struct bt_data ad[] = {
    BT_DATA_BYTES(BT_DATA_FLAGS, (BT_LE_AD_GENERAL | BT_LE_AD_NO_BREDR))
};

void on_bt_ready(int err) {
    bl_le_adv_start(BT_LE_ADV_CONN, ad, ARRAY_SIZE(ad), NULL, NULL);
}

int main() {
    bt_enable(on_bt_ready);
}

For a more complete example you could just try to compile any of the Bluetooth samples as C++ by changing the extension of the files and in CMakeLists.txt. The bluetooth/beacon example is very simple so I suggest starting there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants