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

Ordering between flag and flag_if_supported is not maintained #855

Open
nagisa opened this issue Aug 13, 2023 · 3 comments
Open

Ordering between flag and flag_if_supported is not maintained #855

nagisa opened this issue Aug 13, 2023 · 3 comments

Comments

@nagisa
Copy link
Member

nagisa commented Aug 13, 2023

C and C++ compilers consider the ordering of arguments important and semantic. However, if you construct a builder like this:

cc::Build::new().flag_if_supported("-xc").flag("-xc++").flag("-std=c++11")

the constructed command line will look like this:

$CC -xc++ -std=c++11 -xc ...

which will then error (or warn...), even though the otherwise nonsensical, but valid invocation would be

$CC -xc -xc++ -std=c++11

Some valid use-cases of this ordering generally involve overriding previous arguments.

@nagisa
Copy link
Member Author

nagisa commented Aug 13, 2023

As far as fixing this is concerned, I imagine we would maintain some sort of a per-Builder sequence index that gets associated with each flag pushed to one of the many vectors that are used to maintain these flags.

So instead of flags: Vec<Arc<str>> or flags_supported: Vec<Arc<str>> we'd have flags: Vec<(usize, Arc<str>)>, flags_supported: Vec<(usize, Arc<str>)> and so on for all the flag vectors. Then, in the try_get_compiler function we’d carefully draw from all vectors with flags in the order of usize sequence numbers. This may end up being slower, but with how many flags there are generally, it shouldn’t pose a problem.

We could also replace the many vectors with a vector of enum variants each representing different kind of flag.

Worth noting that changing this would be a breaking change, so it'd be worthwhile to see if we want to or are willing change the behaviour here at all.

@thomcc
Copy link
Member

thomcc commented Aug 14, 2023

You mean it would be breaking to fix the ordering because someone might be relying on us using the current order? I suppose that's true, but I'm not worried about it. We never wrote down that this is the behavior, and this isn't a crate where we'd get anything done if we preserved Hyrum-esque stability promises.

I think I'd take a PR for this if it was reasonably implemented. I don't have a strong opinion between enum vs (usize, Arc<str>), either is fine (the enum approach seems simpler, though). Obviously if the enum approach is used, the type should not be part of the public API.

@nagisa
Copy link
Member Author

nagisa commented Aug 14, 2023

You mean it would be breaking to fix the ordering because someone might be relying on us using the current order?

That too, but especially that now

Build().flag("-bar").flag_if_supported("-foo").flag("-baz")

will test whether appending -foo is valid for $CC -bar -baz rather than for $CC -bar. For some arguments I suspect that might make a difference 🤷 It just isn’t particularly clear to me if people generally just write their flag and flag_if_supporteds out in a randomish order, or if they anticipate that the constructed CLI will roughly match the order of these methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants