refactor: add vec repeat support for logical ops #4998
Conversation
|
There's one more area where we call flux/semantic/semantictest/cmp.go Lines 188 to 201 in 4fb8524 Since this is a part of the testing infra, I'm not sure it matters too much. |
c3b1ea9 to
ad0a904
Compare
cfadabc to
8536ba8
Compare
| @@ -66,6 +66,16 @@ impl Expression { | |||
| fn vectorize(&self, env: &VectorizeEnv<'_>) -> Result<Self> { | |||
| Ok(match self { | |||
| Expression::Identifier(identifier) => { | |||
| let name = identifier.name.clone(); | |||
| if env.config.features.contains(&Feature::VectorizedConst) | |||
| && (name == "true" || name == "false") | |||
There was a problem hiding this comment.
This should check the package of this Symbol so that we know that true/false is coming from universe/prelude. Otherwise this will match on any identifier called true or false.
There was a problem hiding this comment.
Yeah, this makes sense. I'll want to do the same for float. Originally (for float) I tried to check the file field on loc but it was empty for some cases so I removed the check.
If I understand correctly, for the tests this will mean moving things from env to importer so that the package provenance is maintained.
Not sure what's needed to copy stuff over to env to approximate the prelude. Will investigate today.
There was a problem hiding this comment.
Had to work backwards from a regular script run, then make the test setup match how the symbols present in real life. Unexpectedly, the booleans are in the boolean package! I thought they were listed under universe, but no...
During #4622 lots of attention was given to Vector usage in binary ops since the literals that could be vectorized tended to show up there. Since boolean literals were not on the list, I somehow neglected to look at the logical operators (which depend on booleans to be used). This diff adds the necessary branching to cover the vec repeat case, looking forward to the time where boolean literals _can be vectorized_. It also adds skipped/commented out tests with reference to #4997 which should aim to enable these code paths by finally allowing bool literals to become vectorized.
da6ea4c to
a27bc7f
Compare
Also updates supporting test code to emulate package membership and prelude.
Previously we rewrote any call to a function named `float`. This could cause issues if the regular float had been shadowed.
a27bc7f to
6563718
Compare
| let imports: SemanticMap<&str, _> = imp | ||
| .into_iter() | ||
| .map(|(path, pkg)| (path, parse_map(Some(path), pkg))) | ||
| .collect(); | ||
| let importer: Packages = imports | ||
| .into_iter() | ||
| .map(|(path, types)| (path.to_string(), PackageExports::try_from(types).unwrap())) | ||
| .collect(); | ||
| let mut prelude = PackageExports::new(); | ||
| prelude.copy_bindings_from(importer.get("boolean").unwrap()); | ||
| prelude.copy_bindings_from(importer.get("universe").unwrap()); | ||
| let env = Environment::from(&prelude); |
There was a problem hiding this comment.
I cobbled this together based on code I saw in bootstrap -- there might be a more direct/terse way to get this done.
Fixes #4997 #4996
During #4622 lots of attention was given to Vector usage in binary ops
since the literals that could be vectorized tended to show up there.
Since boolean literals were not covered by the initial vec repeat work, I neglected to look
at logical operators (which depend on booleans to be used).
This diff adds the necessary branching to cover the vec repeat case,
looking forward to the time where boolean literals can be vectorizedand also implements support for boolean literals to be vectorized.It also adds skipped/commented out tests with reference to #4997 whichshould aim to enable these code paths by finally allowing bool literals
to become vectorized.
Done checklist