-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Improve Swift support #13120
base: master
Are you sure you want to change the base?
Improve Swift support #13120
Conversation
I.e. types that are not FileOrString.
Essential when a module.modulemap is generated, or any generated header files referenced by a modulemap.
In order to translate “-Wl” into “-Xlinker”.
To avoid warnings about duplicate “-l” flags.
Newer toolchains support it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks nice, and getting some love for Swift is great. There's probably some overlap with #13074 here, and we'll need to figure that out
for arg in dep.get_link_args(): | ||
if arg.startswith("-Wl,"): | ||
for flag in arg[4:].split(","): | ||
result += ["-Xlinker", flag] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's self.LINKER_PREFIX
that should be used here instead of open coding.
@@ -49,6 +50,16 @@ def get_werror_args(self) -> T.List[str]: | |||
def get_dependency_gen_args(self, outtarget: str, outfile: str) -> T.List[str]: | |||
return ['-emit-dependencies'] | |||
|
|||
def get_dependency_link_args(self, dep: 'Dependency') -> T.List[str]: | |||
result = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
result: T.List[str] = []
for flag in arg[4:].split(","): | ||
result += ["-Xlinker", flag] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't right. But calling .split()
here flag becomes a List[str]
, that means that result ends up with ['-Xlinker', ['-linkarg']]
, which is not a string.
seen_libs.add(arg) | ||
else: | ||
deduped_args.append(arg) | ||
return deduped_args |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be the sort of thing that CompileArgs
exists for, is it possible to use that for this purose?
outhandle, outfile = tempfile.mkstemp('.out') | ||
os.close(outhandle) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it not possible to use NamedTemporaryFile
here, to avoid the manual cleanup?
⚔️ conflicts, so needs a rebase. |
(I have two additional fixes that I will submit later, as they depend on APIs added in #12994.)