Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Fix #217 issue with compiling with newer compiler#223

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
jboynes wants to merge1 commit intoarduino:masterfromjboynes:can_msg

Conversation

jboynes
Copy link
Contributor

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

Copy link
Contributor

@aentingeraentinger 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.

@jboynes
Copy link
ContributorAuthor

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
ContributorAuthor

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.oIn 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 online 52 of this file.

@jboynes
Copy link
ContributorAuthor

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
ContributorAuthor

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

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
ContributorAuthor

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

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@aentingeraentingeraentinger left review comments

Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@jboynes@aentinger@per1234

[8]ページ先頭

©2009-2025 Movatter.jp