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

Better descriptor for transitive path #384

Merged
merged 2 commits into from Apr 21, 2021

Conversation

hannahbast
Copy link
Member

The previous descriptor was very cryptic. The reason (probably) was that
neither the subject, nor the predicate, not the object, are directly
known by TransitivePath, but need to be obtained via a bit of
acrobatics. See the code for details.

The previous descriptor was very cryptic. The reason (probably) was that
neither the subject, nor the predicate, not the object, are directly
known by TransitivePath, but need to be obtained via a bit of
acrobatics. See the code for details.
@hannahbast hannahbast requested a review from joka921 April 20, 2021 21:41
Copy link
Member

@joka921 joka921 left a comment

Choose a reason for hiding this comment

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

This is very valuable,
There are 1.5 places where you can save some code which already lies inside the STL.

Comment on lines 81 to 86
auto optional = getIndex().idToOptionalString(_leftValue);
if (optional) {
os << optional.value();
} else {
os << "#" << _leftValue;
}
Copy link
Member

Choose a reason for hiding this comment

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

os << getIndex().idToOptionalString(_leftValue).value_or("#" + std::to_string(_leftValue))
https://en.cppreference.com/w/cpp/utility/optional/value_or

(If you want to, you can make two lines from this, I am sorry, I did not directly tell you this possibility:)

Copy link
Member Author

Choose a reason for hiding this comment

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

Perfect, thanks!

Comment on lines +91 to +94
if (scanOperation != nullptr) {
os << " " << scanOperation->getPredicate() << " ";
} else {
os << " <???> ";
Copy link
Member

Choose a reason for hiding this comment

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

os << " " << scanOperation ? scanOperation->getPredicate() : "<???>" << " "
(only as a possibility, what you wrote is more explicit).

Copy link
Member Author

Choose a reason for hiding this comment

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

I wrote something like that first, but found it harder to read since it spanned two lines and also the ? : has to be put into parantheses. So I would stick with the current version

Comment on lines 101 to 105
if (optional) {
os << optional.value();
} else {
os << "#" << _rightValue;
}
Copy link
Member

Choose a reason for hiding this comment

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

again use value_or (see above)

Copy link
Member Author

Choose a reason for hiding this comment

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

Made the change for this one and the above and tested it and it works fine.

Implemented suggestion from Johannes to the original PR.
Copy link
Member

@joka921 joka921 left a comment

Choose a reason for hiding this comment

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

Thank you very much, this is much more readable in the RuntimeInfo as well as in the code.

@joka921 joka921 merged commit 72fd6f0 into master Apr 21, 2021
@joka921 joka921 deleted the qlever.better-trans-path-descriptor branch April 21, 2021 07:41
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.

None yet

2 participants