-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix: remove preceding -Xcc
instead of the argument following -Werror
or -Wwarning
#9194
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
base: main
Are you sure you want to change the base?
fix: remove preceding -Xcc
instead of the argument following -Werror
or -Wwarning
#9194
Conversation
Sources/Build/BuildDescription/SwiftModuleBuildDescription.swift
Outdated
Show resolved
Hide resolved
case "-Wwarning", "-Werror": | ||
removeNextArg = true | ||
return false | ||
// Remove preceding -Xcc and skip this 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.
Rereading this code again, I think this was actually intended to filter out -Werror SwiftWarningGroup
(which is also valid) rather than -Xcc -Werror
. I think you'll need to examine the preceding arg to check if it's actually -Xcc and therefore safe to remove. If the arg is passed to swiftc directly, the following arg should probably continue to be removed. It would be nice to have some tests for this as well.
Stepping back, this makes me think the compiler should stop diagnosing conflicting flags here and just allow one to win
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.
I think this was actually intended to filter out
-Werror SwiftWarningGroup
Ah, I did not know that. Just found -Werror
in swiftc --help-hidden
.
So, if I understand correctly, the following cases can happen.
-Xcc -Werror
-Xcc -Werror=<CppWarningGroup>
-Werror <SwiftWarningGroup>
and the corresponding Wwarning
variants. Anything missing?
Edit: I suppose the complete list is in this proposal.
For C/C++:
-Wxxxx
-Wno-xxxx
-Werror
-Werror=xxxx
-Wno-error
-Wno-error=xxxx
For Swift:
-warnings-as-errors
-no-warnings-as-errors
-Wwarning xxxx
-Werror xxxx
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.
the compiler should stop diagnosing conflicting flags here and just allow one to win
@owenv If that would be the case, then -suppress-warnings
would have to win out over the others for remote packages. But that might not be a good default for non-remote packages.
When suppressing warnings in remote packages,
SwiftModuleBuildDescription.compileArguments
removes the flag following-Werror
/-Wwarning
instead of the preceding-Xcc
(issue: #9192). This is a proposed fix to that issue by reversing the arguments before filtering.Motivation:
The problem is described in this issue: #9192 .
This was most likely introduced in this PR #8315 .
It looks like the originally submitted code was filtering on the reversed args, in which case the preceding
-Xcc
would have been removed. But the final iteration does not reverse the args.Modifications:
Instead of filtering with a closure with side-effects, initialize an empty array and while iterating over
args
append to new array/skip/skip+remove last as needed.Stale
Reverse
args
before filtering, reverse again after filtering to recover original order. UseArray.lazy
to avoid materializing intermediate arrays.Edit: I'm not completely sure if lazy will play well with the side-effect of mutating the captured
removeNextArg
.Result:
Given
swift build -Xcc -Werror <other flags>
, remote packages should now build equivalently asswift build -suppress-warnings <other flags>
and not throw the internal error.