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

ALL_CAPS are now constants. Changed/shared recursive function environment. Fixed bug with macro environment. Added pprof. #102

Merged
merged 15 commits into from
Aug 5, 2024

Conversation

ldemailly
Copy link
Member

@ldemailly ldemailly commented Aug 4, 2024

fixes #98

#47 (comment)

fixes #47

Also fixes mix up with macro environment which was incorrectly reusing the full eval.State when it really only needs object.Environment, split the two.

This showed up as a bug with ==> macro showing up in output even if no macro was defined (because it got PI and E from other MR)

Added -profile-mem and -profile-cpu options

Figured out regression and saved as examples/pi_perf.gr

Handle tabs in wasm share button

@ldemailly ldemailly marked this pull request as draft August 4, 2024 07:34
@ldemailly ldemailly changed the title ALL_CAPS are now constants - manual tested for all but recursion which is oddly not triggering ALL_CAPS are now constants - also change in recursive function environment Aug 4, 2024
@ldemailly ldemailly marked this pull request as ready for review August 4, 2024 18:37
@ldemailly ldemailly changed the title ALL_CAPS are now constants - also change in recursive function environment ALL_CAPS are now constants - also change in recursive function environment and fix bug with macro environment Aug 4, 2024
@ldemailly ldemailly mentioned this pull request Aug 4, 2024
@ldemailly ldemailly changed the title ALL_CAPS are now constants - also change in recursive function environment and fix bug with macro environment ALL_CAPS are now constants. Changed/shared recursive function environment. Fixed bug with macro environment. Added pprof. Aug 4, 2024
Copy link
Contributor

@ccoVeille ccoVeille left a comment

Choose a reason for hiding this comment

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

I'm happy you have tests, because it is not a fun PR

err := extensions.Init()
if err != nil {
log.Fatalf("Error initializing extensions: %v", err)
return log.FErrf("Error initializing extensions: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

So FErrf is Fatalf ?

The name could be better

Copy link
Member Author

Choose a reason for hiding this comment

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

it's same level as fatal but doesn't try to exit, returns non 0 instead which lets defer run and also make linters happy (and works well with main() {os.Exit(Main())} which lets testscript work well too)

err := extensions.Init()
if err != nil {
log.Fatalf("Error initializing extensions: %v", err)
return log.FErrf("Error initializing extensions: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why using your ferry here, and using Fatalf in previous if block you add

Copy link
Member Author

Choose a reason for hiding this comment

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

?

Copy link
Member Author

@ldemailly ldemailly Aug 5, 2024

Choose a reason for hiding this comment

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

ic you mean why not change all of them, so far I only changed what the linter complained about re defer not to running but probably should be all yes, in theory/for consistency

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks

Copy link
Member Author

Choose a reason for hiding this comment

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

of note there are still a few more log.Fatalf in other functions (like if you fail to pass a valid filename) and it's fine... for now at least (just means some pprof defer won't happen)

@ldemailly ldemailly requested a review from ccoVeille August 5, 2024 14:16
@ldemailly ldemailly merged commit 6b4aac6 into main Aug 5, 2024
1 check passed
@ldemailly ldemailly deleted the constants branch August 5, 2024 14:56
return val
}

// Doesn't unwrap return - return bubbles up.
func (s *State) evalInternal(node any) object.Object {
func (s *State) evalInternal(node any) object.Object { //nolint:funlen // quite a lot of cases.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you could add a map[type]func(Node) Object

Then you create one method per type

So evalInternal method will then simply check if there is a func, then call it

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about this?

Copy link
Member Author

@ldemailly ldemailly Aug 5, 2024

Choose a reason for hiding this comment

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

Thanks for the reminder, I had skipped that one earlier

If it worked, we'd need to profile it (good that there is -profile-cpu now) but I don't think it'd help because you'd need to cast anyway inside each small function (but try and let me know :) ?)

Note that for other types, like token.Type or object.Type an array lookup would be good, as the type fit into uint8 which eliminates bound checks, and I had started something like that in #93

if i != 0 && v == '_' {
continue
}
if v < 'A' || v > 'Z' {
Copy link
Contributor

Choose a reason for hiding this comment

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

Constant may/could also contains number, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

well... not right now indeed :) PI2=PI; PI2++ works https://grol.io/?c=PI2=PI+PI2%2B%2BPI2

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

Successfully merging this pull request may close these issues.

make UPPERCASE constants (you can set once but not change) clarify variable scope, as in...
2 participants