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

Remove unused config.h header from umbrella header directory #23

Conversation

lawrence-forooghian
Copy link
Collaborator

@lawrence-forooghian lawrence-forooghian commented Aug 17, 2022

This fixes a warning that some users were seeing when importing this library as a transitive dependency of ably-cocoa. See commit message for more details.

(I also had to tweak some CI config because it's been so long since we last ran a CI job in this repo.)

Closes #15.

The file include/AblyDeltaCodec.modulemap contains an umbrella header
declaration:

> umbrella header "AblyDeltaCodec/AblyDeltaCodec.h"

According to Clang’s modules documentation
(https://clang.llvm.org/docs/Modules.html):

> An umbrella header includes all of the headers within its directory

> Use the -Wincomplete-umbrella warning option to ask Clang to complain
> about headers not covered by the umbrella header or the module map.

-Wincomplete-umbrella is enabled by default
(https://clang.llvm.org/docs/DiagnosticsReference.html#wincomplete-umbrella).

include/AblyDeltaCodec/AblyDeltaCodec.h doesn’t contain an import of its
neighbouring file config.h (a symlink to a header used internally),
which hence triggers the above warning ("umbrella header for module
'AblyDeltaCodec' does not include header 'config.h'") in apps importing
the AblyDeltaCodec module, i.e. apps importing the AblyDeltaCodec SPM
package.

That config.h symlink is _correctly_ not included in the umbrella header
since it is not part of the public interface of this module. Hence the
correct thing to do is to delete it.

Closes #15.
@lawrence-forooghian lawrence-forooghian force-pushed the remove-unused-headers-from-umbrella-header-directories branch from a9b6939 to 7822a33 Compare August 17, 2022 22:44
Seems like the currently specified versions are no longer available on
the latest GitHub runners.
@lawrence-forooghian lawrence-forooghian force-pushed the remove-unused-headers-from-umbrella-header-directories branch from ed367ef to 7d6ef9d Compare August 17, 2022 22:51
Copy link
Collaborator

@maratal maratal left a comment

Choose a reason for hiding this comment

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

LGTM

@lawrence-forooghian lawrence-forooghian merged commit 21e611a into main Aug 18, 2022
@lawrence-forooghian lawrence-forooghian deleted the remove-unused-headers-from-umbrella-header-directories branch August 18, 2022 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Umbrella header for module 'AblyDeltaCodec' does not include header 'config.h'
2 participants