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

Feature/gcc warning errors #142

Merged
merged 2 commits into from
Nov 7, 2023

Conversation

badevos
Copy link

@badevos badevos commented Nov 6, 2023

Hi,

I'm trying to integrate your library into another project using gcc 8.4.0 with '-Werror'. This gives some small issues. Would it be possible to integrate my changes?

This affects both the library as items into your submodule 'utils'. I create a separate pull request for that.

Logically you can skip the changes to link to my fork, but this allows me to do a full integration myself.

In case you accept my changes I can safely use the upstream version.

Let me know your thoughts.

Thanks in advance.

Kr,
Bart.

@sidcha sidcha self-requested a review November 6, 2023 16:49
@sidcha
Copy link
Member

sidcha commented Nov 6, 2023

Hey! PRs are most welcome and I will be happy to merge any patch that fixes issues in any platform (which looks to be the case here).

In this case, can you please create separate PR in utils submodule first and then update this PR with the submodule bump commit here after the utils changes are merged.

Comment on lines 6 to 8
#ifndef _GNU_SOURCE
#define _GNU_SOURCE /* See feature_test_macros(7) */
#endif
Copy link
Member

Choose a reason for hiding this comment

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

As mentioned in goToMain/c-utils#23, I think we can drop this. Please give it a try and update this PR.

.gitmodules Outdated
@@ -1,3 +1,3 @@
[submodule "utils"]
path = utils
url = https://github.com/goToMain/c-utils.git
url = https://github.com/badevos/c-utils.git
Copy link
Member

Choose a reason for hiding this comment

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

You have to drop this change. You must add a submodule bump commit (after goToMain/c-utils#23 has been merged) instead.

With gcc 8.4.0 the following warning was thrown:

src/osdp_common.c:7: warning: "_GNU_SOURCE" redefined
 #define _GNU_SOURCE  /* See feature_test_macros(7) */

As the application defines using _GNU_SOURCE or not it's safe to
conditionally set this macro
Copy link
Member

@sidcha sidcha left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks for the fixes.

@sidcha sidcha merged commit 2bad5b3 into goToMain:master Nov 7, 2023
9 checks passed
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.

2 participants