Allow opting out of the default umbrella header, to support strict imports #348
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jerrymarino
reviewed
Nov 10, 2021
jerrymarino
approved these changes
Nov 12, 2021
|
@acecilia Hi curious if you folks ever get any performance gain by turning it off? |
gyfelton
added a commit
that referenced
this pull request
Aug 21, 2024
…lt umbrella header generated (#897) ### What changed: 1. When generate_default_umbrella_header is set to False, still add statements around `extern double {module_name}VersionNumber;` to umbrella header so that clang can resolve the situation when module_name is the same for two different modules (where their path is diff). 2. When generate_default_umbrella_header is set to False, use `extern` instead of `FOUNDATION_EXPORT` since `FOUNDATION_EXPORT` is `extern` and trying to define it in `else` clause will cause `warning: ambiguous expansion of macro 'FOUNDATION_EXPORT'` error. ### Why this change: [The original PR](#348) that introduced the flag `generate_default_umbrella_header` essentially wants to not import UIKit/Foundation to restrict on what to import by default, which is a good thing. However, it kinda overkill by also not including the statements around version number and version string. Which apparently affects how clang resolves module mapping (from a module name to a path) and in our set up, leading to error similar to: ``` path/to/alice.modulemap:2:21: error: umbrella for module 'bob' already covers this directory umbrella header "foo-umbrella.h" ^ ``` For reference, this is originated from `diag::err_mmap_umbrella_clash` error in https://github.com/llvm/llvm-project For our case, the umbrella header name for both alice and bob modules are `foo`, which is totally fine since their path is different. But for some reason when trying to compile the code in a remote execution service such as buildfarm, above error shows up. I cannot reproduce it with `local` or `sandboxed` mode. Now looking back, the theory is that when there is clash on umbrealla header name, it will use the module's version string/number to figure out which is which? I don't really have any other theory for now. I cannot find such reference to `VersionNumber`/`VersionString` that is meaningful. Nor can I setup a test case here since it only break remote execution. This does not revert the original PR in anyway, I still get to implicitly import Foundation/UIKit with this change. ### Tests No change on testing since this flag was not tested anywhere for now. I can add this flag to some random testing targets in this PR though but tests so far are still all passing with this turned off.
|
👋 Mmm it has been a long time, but I do not recall observing any major benefit on app size nor build time |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
👋
This PR better supports strict imports, meaning files only import what they use, and they import everything they use.
See documentation in code for the full context 🙏