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

Pass name of the rule to merge / make / etc. #142

Open
bjchambers opened this issue Dec 7, 2021 · 4 comments
Open

Pass name of the rule to merge / make / etc. #142

bjchambers opened this issue Dec 7, 2021 · 4 comments

Comments

@bjchambers
Copy link
Contributor

Sometimes rules panic (due to unexpected cases, etc.). One option would be to have them return an error (#136). Failing that, it would be really helpful if there was a way to get the name of the current rule, so it could be included in the panic message.

Right now, my analysis sometimes panics in cases that shouldn't arise (due to mis-written rewrite rules), and it is tricky to debug since I need to manually use a debugger to get the index of the rule name that is being applied, and then look that up in the symbol table (which isn't exposed, so I had to create a struct with the same layout and then do an unsafe cast to get the index back).

@mwillsey
Copy link
Member

mwillsey commented Dec 9, 2021

This is a good idea. But make and merge are not always called as part of a rule, so we'll have to pass in an option or something.

As an easier workaround, you could log the rule that is currently being applied on this line.

egg/src/run.rs

Line 543 in fbfef11

debug!("Applying {} {} times", rw.name, total_matches);

I think RUST_LOG=egg::run=debug should enable that line if you use env_logger.

@bjchambers
Copy link
Contributor Author

I've noticed that logging in egg (especially with debug) seems very spammy and significantly slows things down, so I've had to turn it down (warning and above only). I could maybe try info, but I'm not sure that logging is a great workaround.

I think passing an option or something should be fine, unless there was a different string/name that could be passed to describe why they're being called.

@bjchambers
Copy link
Contributor Author

Also, the log doesn't seem that useful since the merge isn't applied until the rebuild, which applies all of the unions from all of the previously logged rules.

@mwillsey
Copy link
Member

mwillsey commented Dec 9, 2021

Fair point, the delay makes it less useful.

re: logging. Yes it's a little spammy, but it's good to have more rather than less. You can always target the logging with env_logger. If you have suggestions on turning specific log calls down to lower log levels, that'd be great!

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

No branches or pull requests

2 participants