-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Use an object for option keys #8080
Merged
Merged
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
dcbaker
force-pushed
the
submit/option-key-type
branch
from
December 7, 2020 22:39
42b9250
to
6efed37
Compare
This pull request introduces 7 alerts when merging 6efed37 into e828c67 - view on LGTM.com new alerts:
|
dcbaker
force-pushed
the
submit/option-key-type
branch
from
December 7, 2020 22:53
31744e0
to
3bd598e
Compare
This pull request introduces 1 alert when merging dee56b4 into e828c67 - view on LGTM.com new alerts:
|
dcbaker
force-pushed
the
submit/option-key-type
branch
9 times, most recently
from
December 14, 2020 18:44
81ebdb1
to
e3129ef
Compare
dcbaker
force-pushed
the
submit/option-key-type
branch
from
December 23, 2020 21:49
e3129ef
to
e58fdad
Compare
Ericson2314
approved these changes
Dec 23, 2020
dcbaker
force-pushed
the
submit/option-key-type
branch
from
January 4, 2021 19:22
4bf3928
to
f3df202
Compare
This seems to be in the new C++ module scanner code that is only exercised under Visual Studio 2019 preview. |
This is pretty important to be able to debug the test, as it's huge and really should be a test split into subtests.
Otherwise bugs like "option c_args is unknown" can slip through. that's bad.
This is a complex key that can store multiple bits of data in a single place. It can be generated from a command line formatted string, and it's str method returns it to that form. It's intentionally immutable, use the evolve() method to create variations of an existing key.
that's the only place it's used anyway.
This is useful for figuring out what kind of option this is. My hope is that in the longterm this is less useful, but we'll still want it for the configuration summary printing.
they're probably not strictly needed, but it makes mypy happy.
dcbaker
force-pushed
the
submit/option-key-type
branch
from
January 4, 2021 20:18
f3df202
to
d5880f5
Compare
There's starting to be a lot of things including coredata that coredata needs to itself include. putting it in mesonlib makes more sense
I would have prefered to do these seperatately, but they are combined in some cases, so it was much easier to convert them together. this eliminates the builtins_per_machine dict, as it's duplicated with the OptionKey's machine parameter.
This patches takes the options work to it's logical conclusion: A single flat dictionary of OptionKey: UserOptions. This allows us to simplify a large number of cases, as we don't need to check if an option is in this dict or that one (or any of 5 or 6, actually).
The output of list_targets is a pretty horrific jumble of things. We really need a TypeDict to make this not so terrible we can't deal with it, so for now just use Any.
dcbaker
force-pushed
the
submit/option-key-type
branch
from
January 4, 2021 20:21
d5880f5
to
f14bf8b
Compare
@jpakkane: fixed. Just cygwin failing because pip isn't getting installed. |
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 series introduces a new
OptionKey
type, which is a bit like aNamedTuple
, but with some nice helper methods. The class itself is immutable (or, at least as immutable as a tuple), pickleable, sortable, and comparable. It stores 5 public traits: the name, the target machine, the subproject, the language (if it's a compiler option), and the type (project, base, builtin, compiler, etc). The thing that makes it super nice to work with the theevolve()
method, which takes an existingOptionKey
, and creates a new copy with specific fields overwritten in the copy, ala:It has a few convenience methods for common cases like:
It also provides a
from_string()
alternative constructor, which is parses command line string format, and creates anOptionKey
:Equally importantly, it provides a way back to the string form by converting it to a string:
It also stores it's hash privately (which is safe to do because it's immutable), which makes it very fast to find in dictionaries or hashes.
There is a lot of code churn here, but there's also a lot of really ugly code that goes away as a result, no need for the code that walks over all of the different option dictionaries to figure out where an option is. Nor do we need all of the crazy code to reconstruct the string form from the internal form, and it centralizes the string -> class conversion code in a single place. We also get away from "stringly" typing.
I'm expecting a few failures in CI still (particularly on windows as I haven't run the VS backend yet).