-
Notifications
You must be signed in to change notification settings - Fork 179
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
Issue260 #462
Issue260 #462
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work :) A couple comments.
I would be good with merging this mostly as-is now. And filing an issue for followup work to check more functions. (Maybe you want to work on it, maybe not). Or if you want to work a bit more on this (see above comment), we can leave this open as a WIP.
I moved it to draft so I will correct some simple things. I am not sure what about testing all DB functions for panic (I mean the functions which are unimplemented - see relevant comment). I can try it, but if it is this difficult I assume it is, then I would like some comment if it is worth - maybe it is better to help with "constants or maybe assoc constants?" mentioned by nikomatsakis on design meeting? TBH - anything is ok for me. |
I don't exactly understand why I still see "Changes requested" as I think I changed everything which we talked about (except eternity-estimated tests ;) ), and resolved all conversations - TBH this is my second OpenSource contribution, and preview one was accepted without any comments, so there may be something I don't understand about GitHub review tool... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ended up being a bit verbose. But that's probably fine :) Maybe add a FIXME for someone to followup with actually checking which of these are redundant?
One small comment then I think this is good to land.
One small
chalk-engine/src/logic.rs
Outdated
// Instantiate the table goal with fresh inference variables. | ||
let table_goal = self.tables[table].table_goal.clone(); | ||
let table = Table::<C>::new(goal.clone(), context.is_coinductive(&goal)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just move this to push_intial_strands_instantiated
(or better, just move that function into this one. We don't really need the distinction when this function is 4 lines long.
I think github requests a re-review of approve before changes requested is changed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry two more nits 🙄
tests/integration/panic.rs
Outdated
panicking_method: PanickingMethod, | ||
} | ||
|
||
/// This DB is representint lowered program: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"This DB represents the following lowered program:"
tests/integration/panic.rs
Outdated
/// struct Foo { } | ||
/// trait Bar { } | ||
/// impl Bar for Foo { } | ||
// FIXME: Check if items returned from functions of this struct can be simplified |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently this didn't get saved before I submitted the review:
I would move this to the top of the file above PanickingMethod
and save "FIXME: some of these are probably redundant, so we should figure out which panic in the same place in chalk-engine
"
Worst things that recurring review happened to me on my daily job before, so don't worry ;) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM to me
The test is fixed, so it passes when panic is disabled, and then implementation is fixed - so table is not inserted, if it is not initialized correctly.
I tried to leave initial style, modifying only what is needed to be modified for this to work.
Also @jackh726 mentioned, that maybe after panic there should be two tests - one for goal which might be cached before, and the new one. I would say, that going this direction there could be even three test - one for goal cached before, one for goal on which there was a panic, and one for completely new goal. However I am not very much sure how much benefit would it give, as the fix is actually not to cache thing on panic, but I am ok to implement more tests - I am just worried that
MockDatabase
might grow to size, when it would be unreadable.Fixes #260