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

Fix an issue with bad allocation in post #176

Merged
merged 5 commits into from
Feb 9, 2023
Merged

Fix an issue with bad allocation in post #176

merged 5 commits into from
Feb 9, 2023

Conversation

kilohsakul
Copy link
Collaborator

@kilohsakul kilohsakul commented Feb 8, 2023

Fix of the problem with delta[q] reallocating post and screwing everything.

@Adda0
Copy link
Collaborator

Adda0 commented Feb 8, 2023

I confirm that the tests run successfully on Linux.

@tfiedor tfiedor changed the title Now it works? Really? Pushing for the test to run on linux. Fix an issue with bad allocation in post Feb 8, 2023
@tfiedor
Copy link
Member

tfiedor commented Feb 8, 2023

Is this fix of #173 ?

@Adda0
Copy link
Collaborator

Adda0 commented Feb 8, 2023

Yes, this is a fix for issue #173. However, this implements a different approach that what we have agreed on in our meeting. If we are OK with this approach instead, this PR would resolve #174, indeed.

@Adda0 Adda0 requested review from tfiedor and Adda0 February 8, 2023 12:54
@tfiedor tfiedor linked an issue Feb 8, 2023 that may be closed by this pull request
@tfiedor
Copy link
Member

tfiedor commented Feb 8, 2023

Linked with #173 then

Copy link
Member

@tfiedor tfiedor left a comment

Choose a reason for hiding this comment

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

I have no major issues, I would strongly recommend to protect the static variable with const.

@@ -247,7 +251,20 @@ public:
size_t size() const;


Post & operator[] (State q)
//Post & operator[] (State q)
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth keeping the code commented out?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no, it was a working version, actually I don't know why the reviewers were assigned automatically

Copy link
Collaborator

Choose a reason for hiding this comment

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

I assigned them manually.

@kilohsakul
Copy link
Collaborator Author

Now it should be done, no more modifications planned.

@kilohsakul
Copy link
Collaborator Author

kilohsakul commented Feb 8, 2023

What should actually be the name of that function "mutable_post" ??
Something shorter? Like only delta.mutable(q)? delta.modifiable(q)? delta.modifier(q), ... or whatever else?

@kilohsakul kilohsakul requested a review from jurajsic February 8, 2023 13:59
@Adda0
Copy link
Collaborator

Adda0 commented Feb 8, 2023

I quite like mutable_post, actually. I am open to other suggestions, though.

We could work with something as mut_ref as “mutable reference”. nfa.delta.mut_ref(q) reads naturally to me and it warns us each time we use it that it is mutable and we have to be careful.

Copy link
Collaborator

@Adda0 Adda0 left a comment

Choose a reason for hiding this comment

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

If we are fine with this approach, the implementation seems OK to me.

Copy link
Member

@jurajsic jurajsic left a comment

Choose a reason for hiding this comment

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

It looks fine, if we are sure that the current usages of mutable_post cannot fuck up anything, then we can merge.

@Adda0
Copy link
Collaborator

Adda0 commented Feb 8, 2023

if we are sure that the current usages of mutable_post cannot fuck up anything

In theory, it should not. This PR only makes it more obvious that the underlying structure can be modified. Therefore, one will not use the mutable accessor when they do not want to modify the data structure. Otherwise, using the mutable accessor behaves exactly as it did until now.

@kilohsakul
Copy link
Collaborator Author

Yes, this is a fix for issue #173. However, this implements a different approach that what we have agreed on in our meeting. If we are OK with this approach instead, this PR would resolve #174, indeed.

That's true, but this was really easy, and it looks ok. As I was saying at the meeting, I actually like it this more, but I thought it would be harder to do.

@Adda0
Copy link
Collaborator

Adda0 commented Feb 9, 2023

this was really easy, and it looks ok.

I wholeheartedly agree. I am not against this approach. And as far as I know, I am not aware of any cases where this would not solve our problems.

@Adda0 Adda0 merged commit 37488f7 into devel Feb 9, 2023
@Adda0 Adda0 deleted the repair_delta_box branch February 9, 2023 13:46
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.

Bug in determinize()
4 participants