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.

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

Cooked giga up #123

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

dominik-air
Copy link

@dominik-air dominik-air commented Feb 12, 2025

Description

Related Issue

Resolves #125

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Performance improvement
  • Refactor

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have documented my changes in the code or documentation
  • I have added tests that prove my changes work (if applicable)
  • I have run the unit tests locally
  • I have run the valgrind memory tests locally
  • All new and existing tests pass

@dominik-air dominik-air marked this pull request as draft February 12, 2025 23:01
@araujo88
Copy link
Member

You implemented long and long long as integer types, when they are actually a type modifier. You can have a long double or long long double for example. long float and long long float are not valid types.

@SIGMazer SIGMazer marked this pull request as ready for review February 13, 2025 05:42
@dominik-air
Copy link
Author

lil-yachty-drake

My bad. Gonna rework this no cap

@araujo88
Copy link
Member

Take your time to cook, no rush. And thanks for being willing to grind like a true sigma. Reach out on Discord if you need some guidance on this issue.

@araujo88 araujo88 added the enhancement New feature or request label Feb 13, 2025
@dominik-air dominik-air force-pushed the feat/implement-giga-and-thicc branch from a9037a8 to f49245a Compare February 13, 2025 17:25
Copy link
Contributor

@SIGMazer SIGMazer left a comment

Choose a reason for hiding this comment

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

i think the errors because you edit the code of array access

@dominik-air
Copy link
Author

i think the errors because you edit the code of array access

Yeah, I didn't understand how Value works. Now it's doing what I was expecting

@araujo88
Copy link
Member

@dominik-air do you need some help with this?

@SIGMazer
Copy link
Contributor

@dominik-air You can reduce the scope of the PR to make it just cover one of the modifiers. i think it will be simpler

@araujo88
Copy link
Member

@dominik-air You can reduce the scope of the PR to make it just cover one of the modifiers. i think it will be simpler

Indeed, it is better to keep PRs as granular as possible.

@dominik-air
Copy link
Author

@SIGMazer Agreed. This will be just a giga MR, but I think adding thicc later won't be as challenging.

@araujo88 Help is always appreciated. I kinda tried to refactor some parts to reduce code duplication and this took me a while to implement without new bugs coming up. I still need to work on the "promotion" and node operations for those new types.

@dominik-air dominik-air changed the title Cooked giga and thicc up Cooked giga up Feb 16, 2025
Dockerfile Outdated
@@ -0,0 +1,16 @@
FROM ubuntu:latest
Copy link
Author

Choose a reason for hiding this comment

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

I compile and run brainrot inside a Docker container because I wasn't able to make it work on my Mac. Maybe this could be a future addition to the codebase to ease the setup process. Wdyt @araujo88 @SIGMazer

Of course a separate MR would be better for this contribution. I'll remove this in the final MR

Copy link
Contributor

Choose a reason for hiding this comment

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

yup also i was thinking of add docker container to test the interpreter in other OS

Copy link
Contributor

Choose a reason for hiding this comment

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

btw what the problem in mac os? maybe we can solve it

Copy link
Author

Choose a reason for hiding this comment

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

tbh I don't even remember what was the issue, since I first came across brainrot some weeks ago

I just went with a Docker container because it was less fuss

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair! If you try it and it has an issue please open an issue with the error

Comment on lines -8 to -9
#include <float.h>
#include <stdint.h>
Copy link
Author

Choose a reason for hiding this comment

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

Unused includes

@@ -1081,6 +1118,52 @@ void *handle_unary_expression(ASTNode *node, void *operand_value, int operand_ty
}
}

Value retrieve_array_access_value(ASTNode *node, Value default_return_value)
Copy link
Author

Choose a reason for hiding this comment

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

I wanted to reduce code duplication, so I extracted the array access code into this function

Copy link
Contributor

Choose a reason for hiding this comment

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

about think i was planing to refactor it in some sort of look up table so it will be like this

    get_value[var->type](array_data, idx);

i think this will make it easy for us when implementing pointers and multi-N arrays

Copy link
Author

Choose a reason for hiding this comment

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

yup, good idea, but for now I just wanted to clean up a little bit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for giga type modifier
3 participants