-
Notifications
You must be signed in to change notification settings - Fork 37
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
Allow expressions with type constructors #1313
Conversation
ab4f12b
to
81d72a8
Compare
I'm going to iterate a bit more on this. Turns out we do need to do constant folding at more places to really cover the cases we need. |
85cc2fc
to
b408c1c
Compare
…rval_ns(...)`. So far we used constructor expressions like `interval(...)` to create *constants* of the given type, and for that we required that the argument was a single atomic value. The result was that these constructor expressions were really more like literals for the corresponding types than "normal" expressions. While that's useful internally, it's confusing for the user. This commits is beginning to change that, using `interval`/`interval_NS` as the first instances that now supports arbitrary expressions as its argument. In follow-up commits, we'll port other constructor expressions over to that new syntax. (This also unifies HILTI & Spicy to each provide both of those interval constructors.) To make this change without loosing a way to create actual constants (which we need at some places), we implement this in two stages: the parser initially turns such `interval(...)` expressions into call operators[1] that the types now define for each desired constructor. We then extend the normalizer to look at all the relevant call operators: if they are just receiving a constant value as their argument, then it substitutes them with a plain ``Ctor`` node (``ctor::Interval`` in our cases here). That check for "if they just receive a constant value" isn't quite trivial in general. In our case, where we just want to go back to what we had, it's not that difficult, but more generally it would be nice if we had a constant folder that could compute results even for more complex, but constant, expressions. As that's a piece that our AST infrastructure is still missing, this commit starts going that route by adding a new "constant folder" visitor. Right now, it's covers only the simple cases we need here, but it's something we can extend going forward. Addresses #1123. [1] These call operators are a bit special because they actually don't have anything to call: in `interval(42)` the `interval` isn't any kind of actual value; we just need it for overload resolution. To make that work, we use a `expression::Member` as the first operand, which models a predefined ID. Normally `Member` is just for field lookups in, e.g., structs, but it works just as well elsewhere.
This moves `time/time_ns/stream/error/intN/uintN` over to the new call operator model that we introduced for `interval`. One HILTI-side syntax change: HILTI now also uses `intN(x)` and `uintN(x)` instead of `int<N>`/`uint<N>`. This is to avoid having to introduce two separate set of operators. This change slightly changes timing for when exactly some errors are caught, which required tweaking a couple of tests a bit.
The normalizer now attempts to fold any expression through the constant folder.
Plus, a bit of cleanup.
Note: The original plan was to have this fully replace the optimizer's boolean folding. However, it turns out that that doesn't quite work because it would mess up the optimizer's tracking of feature usage. For now, we special-case the feature constants in the folder and don't touch them.
I believe this path never triggered so far, but it does now with the new constant folding.
The intent has always been to normalize ID scopes so that they aren't prefixed with the current module. However, that wasn't always working because we lost the state of the current scope. Now moving that into a global to retain it across different printing streams.
17a5676
to
a44ecdc
Compare
f71f2e2
to
78ae64b
Compare
@bbannier This is now ready for review again. The changes are in commits on top of the previous version, hope that works. |
I'm going to hold off merging this for a little bit more to not block CI, assuming, @bbannier, you're cutting a release. |
@@ -3,7 +3,7 @@ | |||
url = https://github.com/onqtam/doctest.git | |||
[submodule "3rdparty/SafeInt"] | |||
path = 3rdparty/SafeInt | |||
url = https://github.com/dcleblanc/SafeInt | |||
url = https://github.com/rsmmr/SafeInt.git |
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.
@rsmmr, do you plan to upstream your patch? Also, using a private fork is not ideal as it makes pulling in upstream fixes impossible for anyone but you.
No description provided.