Handle -Xarch_<arch> <arg> for Clang #2265
Conversation
| ArgInfo::TakeArg(_s, create, ArgDisposition::ConcatenatedAndSeparated(_)) => { | ||
| if let Some(a) = get_next_arg() { | ||
| Argument::WithValue( | ||
| arg.to_string().leak(), |
There was a problem hiding this comment.
I'm not a Rust developer, so hopefully there is a better way to pass arg here than to leak it 😅
There was a problem hiding this comment.
it would have been nice to investigate this before submitting the PR ;)
There was a problem hiding this comment.
Believe me, I did. This is what I came up with in the end, and figured pushing a WIP so someone more knowledgeable in Rust could provide input. Would you rather I had given up on the patch and never submitted a PR instead because I couldn't come up with anything better? 🤷♂️
There was a problem hiding this comment.
Or did you mean before landing/pushing it into master? If so, yes, we definitely want to solve that first 😅
| @@ -176,6 +176,7 @@ counted_array!(pub static ARGS: [ArgInfo<gcc::ArgData>; _] = [ | |||
| take_arg!("-MF", PathBuf, CanBeSeparated, DepArgumentPath), | |||
| take_arg!("-MQ", OsString, CanBeSeparated, DepTarget), | |||
| take_arg!("-MT", OsString, CanBeSeparated, DepTarget), | |||
| take_arg!("-Xarch", OsString, ConcatenatedAndSeparated('_'), PassThrough), | |||
There was a problem hiding this comment.
Is PassThough the right choice here? I only dealt with the mechanism of recognizing and processing the arg, but not the semantics of passing it and how that may affect the cache.
|
thanks |
|
The test suite still passes for me locally, but I assume we want some test coverage for this as well. |
|
I extended |
A new ArgDisposition is introduces that combines the Concatenated and Separated behaviors, but produces results similar to a plain Separated argument with the full original -Xarch_<arch> argument. Fixes mozilla#2090
i don't see it ? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2265 +/- ##
==========================================
+ Coverage 30.91% 40.65% +9.74%
==========================================
Files 53 54 +1
Lines 20112 20722 +610
Branches 9755 9628 -127
==========================================
+ Hits 6217 8424 +2207
- Misses 7922 8142 +220
+ Partials 5973 4156 -1817 ☔ View full report in Codecov by Sentry. |
| let info = take_arg!("-foo", OsString, ConcatenatedAndSeparated('_'), Foo); | ||
| assert_eq!( | ||
| info.clone().process("-foo_bar", || None).unwrap_err(), | ||
| ArgParseError::UnexpectedEndOfArgs | ||
| ); | ||
| assert_eq!( | ||
| info.process("-foo_bar", || Some("baz".into())).unwrap(), | ||
| arg!(WithValue("-foo_bar", Foo("baz"), Separated)) | ||
| ); |
Do you have an example of an existing test I can base it on? Not sure where to start |
|
Ping :) |
|
see some examples here :) |
A new ArgDisposition is introduces that combines the Concatenated and Separated behaviors, but produces results similar to a plain Separated argument with the full original -Xarch_ argument.
Fixes #2090