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

Introduce HashMap for Symbol Table Management #78

Merged
merged 2 commits into from
Jan 18, 2025
Merged

Conversation

SIGMazer
Copy link
Contributor

Description

  • Replaced the existing symbol table implementation with a HashMap for faster lookups.
  • Updated all functions interacting with symbol_table to accommodate the new HashMap structure.

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

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)
  • All new and existing tests pass

@SIGMazer SIGMazer added the refactor Modify existing working code but keeping the same functionality. label Jan 17, 2025
@SIGMazer SIGMazer force-pushed the refactor/symbol_table branch from 3de3ecf to 4aa9efb Compare January 17, 2025 01:35
@araujo88
Copy link
Member

I've made a hashmap lib in C a while ago, might give it a shot? https://github.com/araujo88/chashmap

@SIGMazer SIGMazer force-pushed the refactor/symbol_table branch from 4aa9efb to 61e3fba Compare January 17, 2025 02:05
@SIGMazer
Copy link
Contributor Author

I've made a hashmap lib in C a while ago, might give it a shot? https://github.com/araujo88/chashmap

sure

@araujo88
Copy link
Member

araujo88 commented Jan 17, 2025

I've made a hashmap lib in C a while ago, might give it a shot? https://github.com/araujo88/chashmap

sure

from what I've seen, the implementation is similar. We just have to make sure that it is as safe as possible and avoid memory leaks. Perhaps running some tests with valgrind?

@SIGMazer
Copy link
Contributor Author

I've made a hashmap lib in C a while ago, might give it a shot? https://github.com/araujo88/chashmap

sure

from what I've seen, the implementation is similar. We just have to make sure that it is as safe as possible and avoid memory leaks. Perhaps running some tests with valgrind?

i checked it and i found set variables function leaks memory and i fix it in last commit but also create_node some how leak memory and i cannot get the cause

==709571== 224 bytes in 4 blocks are definitely lost in loss record 62 of 71
==709571==    at 0x484280F: malloc (vg_replace_malloc.c:442)
==709571==    by 0x4066F5: create_node (in /home/sigmazer/projects/brainrot/brainrot)
==709571==    by 0x4067AA: create_int_node (in /home/sigmazer/projects/brainrot/brainrot)
==709571==    by 0x402C0B: yyparse (in /home/sigmazer/projects/brainrot/brainrot)
==709571==    by 0x403535: main (in /home/sigmazer/projects/brainrot/brainrot)

@SIGMazer SIGMazer force-pushed the refactor/symbol_table branch from 834ced0 to 1869661 Compare January 17, 2025 04:06
@SIGMazer SIGMazer force-pushed the refactor/symbol_table branch from 1869661 to 964be8b Compare January 17, 2025 04:07
@araujo88
Copy link
Member

I've made a hashmap lib in C a while ago, might give it a shot? https://github.com/araujo88/chashmap

sure

from what I've seen, the implementation is similar. We just have to make sure that it is as safe as possible and avoid memory leaks. Perhaps running some tests with valgrind?

i checked it and i found set variables function leaks memory and i fix it in last commit but also create_node some how leak memory and i cannot get the cause

==709571== 224 bytes in 4 blocks are definitely lost in loss record 62 of 71
==709571==    at 0x484280F: malloc (vg_replace_malloc.c:442)
==709571==    by 0x4066F5: create_node (in /home/sigmazer/projects/brainrot/brainrot)
==709571==    by 0x4067AA: create_int_node (in /home/sigmazer/projects/brainrot/brainrot)
==709571==    by 0x402C0B: yyparse (in /home/sigmazer/projects/brainrot/brainrot)
==709571==    by 0x403535: main (in /home/sigmazer/projects/brainrot/brainrot)

I was thinking of adding a valgrind check to the CI. It would run on every test we have. What do you think?

@SIGMazer
Copy link
Contributor Author

That will be nice

@SIGMazer
Copy link
Contributor Author

I've made a hashmap lib in C a while ago, might give it a shot? https://github.com/araujo88/chashmap

sure

from what I've seen, the implementation is similar. We just have to make sure that it is as safe as possible and avoid memory leaks. Perhaps running some tests with valgrind?

i checked it and i found set variables function leaks memory and i fix it in last commit but also create_node some how leak memory and i cannot get the cause

==709571== 224 bytes in 4 blocks are definitely lost in loss record 62 of 71
==709571==    at 0x484280F: malloc (vg_replace_malloc.c:442)
==709571==    by 0x4066F5: create_node (in /home/sigmazer/projects/brainrot/brainrot)
==709571==    by 0x4067AA: create_int_node (in /home/sigmazer/projects/brainrot/brainrot)
==709571==    by 0x402C0B: yyparse (in /home/sigmazer/projects/brainrot/brainrot)
==709571==    by 0x403535: main (in /home/sigmazer/projects/brainrot/brainrot)

I was thinking of adding a valgrind check to the CI. It would run on every test we have. What do you think?

will we fix the memory leaks in this pr or merge it and make a new one with valgrind check?

@araujo88
Copy link
Member

araujo88 commented Jan 18, 2025

I've made a hashmap lib in C a while ago, might give it a shot? https://github.com/araujo88/chashmap

sure

from what I've seen, the implementation is similar. We just have to make sure that it is as safe as possible and avoid memory leaks. Perhaps running some tests with valgrind?

i checked it and i found set variables function leaks memory and i fix it in last commit but also create_node some how leak memory and i cannot get the cause

==709571== 224 bytes in 4 blocks are definitely lost in loss record 62 of 71
==709571==    at 0x484280F: malloc (vg_replace_malloc.c:442)
==709571==    by 0x4066F5: create_node (in /home/sigmazer/projects/brainrot/brainrot)
==709571==    by 0x4067AA: create_int_node (in /home/sigmazer/projects/brainrot/brainrot)
==709571==    by 0x402C0B: yyparse (in /home/sigmazer/projects/brainrot/brainrot)
==709571==    by 0x403535: main (in /home/sigmazer/projects/brainrot/brainrot)

I was thinking of adding a valgrind check to the CI. It would run on every test we have. What do you think?

will we fix the memory leaks in this pr or merge it and make a new one with valgrind check?

I think fixing the memory leaks would be beyond the scope of this PR. We can merge as it is and then tackle the memory leaks in a separate PR. I'll create a PR adding a memory management lib.

@SIGMazer SIGMazer merged commit aae6e62 into main Jan 18, 2025
2 checks passed
@SIGMazer SIGMazer deleted the refactor/symbol_table branch January 18, 2025 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Modify existing working code but keeping the same functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants