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

Fix #217 issue with compiling with newer compiler #223

Closed
wants to merge 1 commit into from

Conversation

jboynes
Copy link
Contributor

@jboynes jboynes commented Nov 2, 2023

See comment on #217 about problem compiling tests on desktop (specifically with clang 15.0.0)

Copy link
Contributor

@aentinger aentinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be a problem for you, but directly invoking the Constructor could invoke a problem for the Arduino cores using this API.

@aentinger aentinger closed this Nov 3, 2023
@jboynes
Copy link
Contributor Author

jboynes commented Nov 3, 2023

Please can you clarify the potential problem you see here so I can fix this another way.

Are you concerned about the use of a delegating constructor like is used on line 52?

@aentinger
Copy link
Contributor

Are you concerned about the use of a delegating constructor like is used on line 52?

Yes.

Also, you are probably the only one that compiles the unit tests with clang. I'm reluctant to make this change to just accomodate a single user (sorry, but we've got to keep the bigger picture in mind, and clang is more exotic than plain old GCC).

@aentinger
Copy link
Contributor

If you can fix your issue with #pragmas, that would also be a way. But no ctor delegation, sorry.

@jboynes
Copy link
Contributor Author

jboynes commented Nov 3, 2023

The problem here isn't specific to clang but more to do with newer versions of the compilers. I get the same error with GCC-13

[  1%] Building CXX object CMakeFiles/test-ArduinoCore-API.dir/src/CanMsg/test_CanMsg.cpp.o
In file included from .../ArduinoCore-API/test/src/CanMsg/test_CanMsg.cpp:11:
.../ArduinoCore-API/test/../api/CanMsg.h: In copy constructor 'arduino::CanMsg::CanMsg(const arduino::CanMsg&)':
.../ArduinoCore-API/test/../api/CanMsg.h:58:36: error: the address of 'arduino::CanMsg::data' will never be NULL [-Werror=address]
   58 |     if (this->data_length && other.data)
      |                              ~~~~~~^~~~

The warning is correct, other.data can never be null because it's an array member and not a pointer, and warnings are treated as fatal errors.

So the simple fix here is just to remove the redundant check and I can update the PR to do that.

However, I'm unclear on the concern with delegating constructors: they are a C++11 feature which is the language version used by the API, and they are already used elsewhere in the code, for example on line 52 of this file.

@jboynes
Copy link
Contributor Author

jboynes commented Nov 3, 2023

Branch updated not to use constructor delegation. Should I create a new PR or would you reopen this one?

@aentinger
Copy link
Contributor

Should I create a new PR or would you reopen this one?

Just reopen this PR.

@jboynes
Copy link
Contributor Author

jboynes commented Nov 3, 2023

Should I create a new PR or would you reopen this one?

Just reopen this PR.

I don't see the buttons for that. I'm guessing I don't have permission and so it would need to be reopened by committer.

@per1234
Copy link
Collaborator

per1234 commented Nov 3, 2023

It looks like it is impossible to reopen this pull request due to changes to the head branch:

image

So it will be necessary to open a new PR.

@jboynes
Copy link
Contributor Author

jboynes commented Nov 3, 2023

Thanks for checking. I rebased and so had to force push, I guess that broke it.
Reopened as #224

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

Successfully merging this pull request may close these issues.

3 participants