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.

Sign up for GitHub

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

Jump to bottom

feat: do compile time math for range neq #469

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

feat: do compile time math for range neq #469

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
762f323
feat: do compile time math for range neq
b1ek Sep 7, 2024
675e938
Update src/modules/expression/binop/range.rs
b1ek Sep 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/modules/expression/binop/range.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use heraclitus_compiler::prelude::*;
use crate::docs::module::DocumentationModule;
use crate::modules::expression::expr::ExprType;
use crate::{handle_binop, error_type_match};
use crate::modules::{expression::expr::Expr, types::{Type, Typed}};
use crate::utils::metadata::ParserMetadata;
Expand Down Expand Up @@ -59,7 +60,11 @@ impl TranslateModule for Range {
let from = self.from.translate(meta);
let to = self.to.translate(meta);
if self.neq {
let to_neq = translate_computation(meta, ArithOp::Sub, Some(to), Some("1".to_string()));
let to_neq = match &self.to.value.as_ref().unwrap() {
ExprType::Number(_) => (to.parse::<i64>().unwrap() - 1_i64).to_string(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any specific reason to use i64 over isize?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope. does it matter?

Copy link
Member

@mks-h mks-h Sep 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, a little. The isize type is adaptive to any bitness of the target architecture — it will be 32-bit on a 32-bit machine, and 64-bit on a 64-bit machine. The i64 type can be made to work on a 32-bit arch, but it will have a performance overhead.

I know we aren't really targeting 32-bit machines, but it won't hurt to keep the codebase a little bit more universal. Especially if it is this simple to do.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's precisely the reason why I use usize instead of u64 everywhere in the Amber compiler and Heraclitus

ExprType::VariableGet(_) => translate_computation(meta, ArithOp::Sub, Some(to), Some("1".to_string())),
_ => unreachable!("range expression must be either Number or VariableGet"),
Comment on lines +65 to +66
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not that simple, we should be able to do this kind of things:

0..(var + 10)

just instead of specifing VariableGet as the only acceptable expression other than Number, allow for any other expression. Compiler checks if it has correct Num type, so we're safe here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly. @b1ek look at the line 13 where a struct is defined. We have left and right arm that are expressions:

#[derive(Debug, Clone)]
pub struct Range {
    from: Box<Expr>,
    to: Box<Expr>,
    neq: bool
}

We can't narrow the arms to variable get and number literal. This should literally expect any expression just like before we did.

};
meta.gen_subprocess(&format!("seq {} {}", from, to_neq))
} else {
meta.gen_subprocess(&format!("seq {} {}", from, to))
Expand Down