Support builtin functions #469

ghaith · 2022-02-21T09:54:13Z

Add information of builtin to the index
Builtins are hardcoded in the builtin module
Implented REF and ADR

codecov-commenter · 2022-02-21T10:04:12Z

Codecov Report

Merging #469 (a4cbbe4) into master (fd520d7) will increase coverage by 92.76%.
The diff coverage is 91.30%.

❗ Current head a4cbbe4 differs from pull request most recent head f207776. Consider uploading reports for the commit f207776 to get more accurate results

@@             Coverage Diff             @@
##           master     #469       +/-   ##
===========================================
+ Coverage        0   92.76%   +92.76%     
===========================================
  Files           0       37       +37     
  Lines           0    13330    +13330     
===========================================
+ Hits            0    12366    +12366     
- Misses          0      964      +964

Impacted Files	Coverage Δ
src/ast.rs	`92.82% <ø> (ø)`
src/codegen.rs	`100.00% <ø> (ø)`
src/index/const_expressions.rs	`87.50% <70.00%> (ø)`
src/builtins.rs	`78.37% <78.37%> (ø)`
src/codegen/generators/expression_generator.rs	`86.54% <95.00%> (ø)`
src/index.rs	`98.13% <100.00%> (ø)`
src/index/visitor.rs	`96.35% <100.00%> (ø)`
src/lib.rs	`74.78% <100.00%> (ø)`
src/resolver.rs	`96.45% <100.00%> (ø)`
src/test_utils.rs	`100.00% <100.00%> (ø)`
... and 37 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd520d7...f207776. Read the comment docs.

riederm · 2022-02-21T10:44:14Z

src/codegen/generators/expression_generator.rs

@@ -449,6 +450,19 @@ impl<'a, 'b> ExpressionCodeGenerator<'a, 'b> {
                )
            })?;

+        //If the function is builtin, generate a basic value enum for it
+        if self.index.is_builtin(implementation.get_call_name()) {


I find it clearer, if the index would return you an Option that you call ...

riederm · 2022-02-21T10:47:54Z

src/index.rs

@@ -426,6 +428,11 @@ pub struct Index {
 }

 impl Index {
+    pub fn create_with_builtins(id_provider: IdProvider) -> Index {


explain in documentation what this means

riederm · 2022-02-21T10:51:36Z

src/index/visitor.rs

-pub fn visit(unit: &CompilationUnit, mut id_provider: IdProvider) -> Index {
-    let mut index = Index::default();
-
+pub fn visit_index(mut index: Index, unit: &CompilationUnit, mut id_provider: IdProvider) -> Index {


the index passed here is always Index::default so we could remove it again

The builtinin index is now created first in the lib, and not always by visiting an ast. Also fixed the index import to use not skip enum and hardware constants. As a side effect of the index being imported, some index numbers changed in some tests

riederm

it feels that you merged two issues here together?
is this whole import-code related to #469?
do you think you can get this out into it's own commit?

riederm · 2022-03-03T14:14:45Z

src/builtins.rs

+}
+
+pub struct BuiltIn {
+    decl: &'static str,


so far we avoided &'static str
is there a benefit over String?

Honestly, I don't remember, it feels like less code (not having .to_string() in each entry)
I think initially this came when I was using a static array of declarations instead of the lazy_static map

riederm · 2022-03-03T14:19:43Z

src/index.rs

+        //enmu_variables use the qualified variables since name conflicts will be overriden in the enum_global
+        for (qualified_name, e) in other.enum_qualified_variables.drain(..) {
+            let e = self.transfer_constants(e, &mut other.constant_expressions);
+            dbg!(&e);


remove dbg!
btw. is this code for another issue? or is it related to builtins?

Since we agreed to not have the builtins sneaked into the index when it's created but have them be parsed independently the import issue has to be solved for the builtins to work.
I could still split the issue in 2 but the import has to come first

riederm · 2022-03-03T14:21:43Z

src/index.rs

@@ -513,7 +544,7 @@ impl Index {
    ) -> Option<ConstId> {
        initializer_id
            .as_ref()
-            .and_then(|it| import_from.remove(it))
+            .and_then(|it| import_from.clone(it))


was this a test? or is this really required for builtins?

AFAIK it is required, the problem is that we reused IDs for different types, and if we delete one when importing it is lost to the others who used it.
Since we own the other index, it will be dropped/deleted anyway

riederm · 2022-03-03T14:22:07Z

src/index.rs

@@ -504,6 +503,38 @@ impl Index {
        // self.constant_expressions.import(other.constant_expressions)
    }

+    fn transfer_constants(


related to builtins?

Yes, see above

riederm · 2022-03-03T14:23:13Z

src/index/const_expressions.rs

-        self.expressions.remove(*id).map(|it| match it.expr {
-            ConstExpression::Unresolved { statement, scope } => {
-                (statement, it.target_type_name, scope)
+    // /// removes the expression from the ConstExpressions and returns all of its elements


this whole code is probably related to a different issue?

riederm · 2022-03-03T14:24:32Z

src/lib.rs

    let mut units = Vec::new();

+    //parse the builtins into the index
+    let (builtins, _) = builtins::parse_built_ins(id_provider.clone());


if the second element of the tuple is not used here, do we ever use it?

No, I don't assume our own code will return diagnostics, I can make the method return only the compilation unit

riederm

i think this can find its way into master

ghaith added 2 commits February 21, 2022 09:45

Support builtin functions

c5a5c28

Add correctness tests

cfc603f

ghaith requested a review from riederm February 21, 2022 09:54

riederm requested changes Feb 21, 2022

View reviewed changes

ghaith force-pushed the builtin_functions branch 3 times, most recently from 50c2874 to 99e8179 Compare February 24, 2022 07:54

Review comments

f9b9dc4

The builtinin index is now created first in the lib, and not always by visiting an ast. Also fixed the index import to use not skip enum and hardware constants. As a side effect of the index being imported, some index numbers changed in some tests

ghaith force-pushed the builtin_functions branch from 99e8179 to f9b9dc4 Compare February 24, 2022 07:55

riederm requested changes Mar 3, 2022

View reviewed changes

ghaith added 2 commits March 4, 2022 07:42

Review Comments

a4cbbe4

Merge branch 'master' into builtin_functions

159fa90

ghaith requested a review from riederm March 9, 2022 07:56

ghaith added 2 commits March 18, 2022 10:30

Run clippy and fmt

84eddfc

Merge branch 'master' into builtin_functions

f207776

riederm approved these changes Apr 11, 2022

View reviewed changes

ghaith merged commit 2e3ebff into master Apr 11, 2022

ghaith deleted the builtin_functions branch April 11, 2022 09:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support builtin functions #469

Support builtin functions #469

ghaith commented Feb 21, 2022 •

edited

Loading

codecov-commenter commented Feb 21, 2022 •

edited

Loading

riederm Feb 21, 2022

riederm Feb 21, 2022

riederm Feb 21, 2022

riederm left a comment

riederm Mar 3, 2022

ghaith Mar 4, 2022

riederm Mar 3, 2022

ghaith Mar 4, 2022

riederm Mar 3, 2022

ghaith Mar 4, 2022

riederm Mar 3, 2022

ghaith Mar 4, 2022

riederm Mar 3, 2022

riederm Mar 3, 2022

ghaith Mar 4, 2022

riederm left a comment

Support builtin functions #469

Support builtin functions #469

Conversation

ghaith commented Feb 21, 2022 • edited Loading

codecov-commenter commented Feb 21, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riederm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riederm left a comment

Choose a reason for hiding this comment

ghaith commented Feb 21, 2022 •

edited

Loading

codecov-commenter commented Feb 21, 2022 •

edited

Loading