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

Unit test for checking if table view from link list can be sync'ed #1390

Closed
wants to merge 1 commit into from

Conversation

kneth
Copy link
Contributor

@kneth kneth commented Dec 9, 2015

Translation of realm/realm-java#1886 to core/C++ that is, it is as close to how the Java binding is implemented as possible. A seg. fault happens in sync_if_needed().

Question to @realm/core: Is the Java binding doing something wrong?

Fun fact: Merging in #1392 does not resolved the issue.

LangBindHelper::promote_to_write(sg, *hist);
TableRef table1 = g.get_table("table1");
TableView view = table1->where().find_all();
Query query = static_cast<Query>(view.get_query()).get_table()->where(table1->get_linklist(0, 0));
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this?

Copy link
Contributor

Choose a reason for hiding this comment

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

static_cast, then assignment? Is core supposed to support this use?

Copy link
Contributor

Choose a reason for hiding this comment

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

Struck me as suspicious too — a lot of methods on Query return Query&, so this is at high risk of operating on either temporaries or modifying the underlying Query for the TableView.

Copy link
Contributor

Choose a reason for hiding this comment

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

TableView::get_query returns a const Query, which this static_cast casts away. An equivalent form would be:

    Query query = const_cast<Query*>(&view.get_query())->get_table()->where(table1->get_linklist(0, 0));

Whether that's legal or not is another matter.

Copy link
Contributor

Choose a reason for hiding this comment

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

It must be copied then, because afaik static_cast cannot touch constness.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, ::get_table() is not const.

Copy link
Contributor

Choose a reason for hiding this comment

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

static_cast indeed cannot cast away constness. So this line is correct, even if it's written in a convoluted way. :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should change our API to minimize this kind of usage if possible. Why is there even a get_query() on a TableView?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to what @finnschiermer said

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Link query on a table view.

@finnschiermer
Copy link
Contributor

Why do you need to use get_query on the table view. Do you usually do that?

@kneth
Copy link
Contributor Author

kneth commented Dec 11, 2015

You can't query a table view directly so you can to create a new query using the table view as a filter. Instead if get_query().get_table(), you could do get_parent().

@zaki50
Copy link
Contributor

zaki50 commented Dec 11, 2015

Java code is:
object.getColumnRealmList().where().findAll();

To minimize the code, there are no conditions now after where() but there are some conditions usually.

@rrrlasse
Copy link
Contributor

@kneth I found the real cause of the crash now (it wasn't a bug in move_last_over() anyway). It occurs because your query is refering to a LinkList that you deleted earlier:

Query query = static_cast<Query>(view.get_query()).get_table()->where(table1->get_linklist(0, 0)); ... table1->move_last_over(0); ... view2.sync_if_needed();

And sure, the code could be beautified and get_query() is also error prone to use (TableView may depend on a chain of further older queries, or may originate from something else than a Query, etc, etc).

@kneth
Copy link
Contributor Author

kneth commented Dec 14, 2015

@rrrlasse Why does is_attached() then return true?

@kneth
Copy link
Contributor Author

kneth commented Dec 16, 2015

@rrrlasse Will you create an issue about is_attached() should return false? When you do, you can either use this PR as a unit test or just close it.

@kneth
Copy link
Contributor Author

kneth commented Dec 16, 2015

I have created #1413 and closing this one.

@kneth kneth closed this Dec 16, 2015
@kneth kneth removed the P1 label Dec 16, 2015
@teotwaki teotwaki deleted the kg/bug/tableview-clear branch May 23, 2016 09:47
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants