Skip to content

pkg/gecko_sdk: fix #8266#8487

Closed
pyropeter wants to merge 1 commit intoRIOT-OS:2018.01-branchfrom
beduino-project:fix-issue-8266
Closed

pkg/gecko_sdk: fix #8266#8487
pyropeter wants to merge 1 commit intoRIOT-OS:2018.01-branchfrom
beduino-project:fix-issue-8266

Conversation

@pyropeter
Copy link
Copy Markdown
Contributor

Contribution description

Fix fallthrough errors for GCC 7.x (#8265).

Issues/PRs references

Fixes #8266

@smlng smlng added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 30, 2018
$(error This package can only be used with EFM32 CPUs)
endif

CFLAGS += -Wno-error=implicit-fallthrough
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

minor nit: we typically use a different notation: -Wno-implicit-fallthrough

Copy link
Copy Markdown
Member

@jnohlgard jnohlgard Jan 30, 2018

Choose a reason for hiding this comment

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

@smlng

minor nit: we typically use a different notation: -Wno-implicit-fallthrough

I would prefer if the error is disabled rather than hiding the warning entirely.

Edit: added reference to what this was meant as a reply to.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

good point, make sense to retain the warning

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is even more important in this package, as the upstream software is mostly used in RIOT. So if we hide the warning in RIOT, this will never be fixed upstream.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

agreed

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

on the other hand: murdock doesn't like it, at least there is an error.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Compiler too old?

Copy link
Copy Markdown
Member

@smlng smlng Jan 31, 2018

Choose a reason for hiding this comment

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

well we do have a range of different, platform dependent compilers of various version - hence we need a solution that works for a considerable number of them. That means, GCC 5, 6, and 7 for instance; and without much if-else stuff.

I agree it would be best to have the warnings but if that doesn't work we should fall-back to use -Wno-implicit-fallthrough as we do for in (many) other cases.

@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Feb 5, 2018

@basilfx you might be interested as you maintain the package repository.

@kYc0o
Copy link
Copy Markdown
Contributor

kYc0o commented Feb 13, 2018

@pyropeter can you address Murdock errors and rebase?

@kYc0o
Copy link
Copy Markdown
Contributor

kYc0o commented Feb 13, 2018

@pyropeter can you address Murdock errors

Ok I just read that's because Murdock's GCC is old. So I think to patch it upstream it's a better idea.

@basilfx
Copy link
Copy Markdown
Member

basilfx commented Feb 13, 2018

I've opened #8554 to fix #8266 via upstream.

@basilfx
Copy link
Copy Markdown
Member

basilfx commented Feb 13, 2018

Closing this issue in favor of #8554 (merged).

@basilfx basilfx closed this Feb 13, 2018
@jnohlgard jnohlgard mentioned this pull request Feb 19, 2018
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants