Skip to content

Add optional_lookup utility function #3473

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

Conversation

hannes-steffenhagen-diffblue
Copy link
Contributor

@hannes-steffenhagen-diffblue hannes-steffenhagen-diffblue commented Nov 27, 2018

This provides a more ergonomic interface over map::find for the case when you don't need an iterator

  • Each commit message has a non-empty body, explaining why the change was made.
  • Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
  • The feature or user visible behaviour I have added or modified has been documented in the User Guide in doc/cprover-manual/
  • Regression or unit tests are included, or existing tests cover the modified code (in this case I have detailed which ones those are in the commit message).
  • My commit message includes data points confirming performance improvements (if claimed).
  • My PR is restricted to a single feature or bugfix.
  • White-space or formatting changes outside the feature-related changed lines are in commits of their own.

Copy link
Contributor

@xbauch xbauch left a comment

Choose a reason for hiding this comment

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

Looks good, would be nice to generalise it to use std::find in the general case. Then a specialisation for containers that implement their own find function.

@hannes-steffenhagen-diffblue
Copy link
Contributor Author

hannes-steffenhagen-diffblue commented Nov 27, 2018

@xbauch Thought about it, wasn't completely sure if it was a good idea because for most containers the result should be optional<value_type>, except for map where we only want the value but value_type is a key-value pair... Could certainly implement it like that, just worried that it might be confusing.

@hannes-steffenhagen-diffblue hannes-steffenhagen-diffblue force-pushed the feature-optional_map_lookup branch 2 times, most recently from 98e0a72 to 5139616 Compare November 27, 2018 17:56
Copy link
Contributor

@allredj allredj left a comment

Choose a reason for hiding this comment

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

✔️
Passed Diffblue compatibility checks (cbmc commit: 5139616).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/92738933

@sonodtt
Copy link
Contributor

sonodtt commented Nov 28, 2018

Interesting. To follow on from @xbauch 's comment - it seems that as a generic problem, this becomes quite non-trivial.
As I understand it, specialisation in the generic scenario "has a method .find()" would involve the member detection idiom. Can define a trait for checking for find(). Quite an issue to make robust considering inherited methods et al.
But essentially a form of contract.

In the non generic scenairo an overload for std::map or other known types.

These guys, who quite definitely know what they're doing - found a simple edge case, that seemed surprisingly hard to get right:
https://web.archive.org/web/20180409123554/https://blog.quasardb.net/sfinae-hell-detecting-template-methods

@sonodtt sonodtt self-requested a review November 28, 2018 02:14
Copy link
Collaborator

@tautschnig tautschnig left a comment

Choose a reason for hiding this comment

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

This strictly needs a unit test. At present, we don't even know whether this is syntactically acceptable to all our compilers used in CI.

#include "optional.h"

#include <iterator>
#include <map>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are those includes required here?

@hannes-steffenhagen-diffblue
Copy link
Contributor Author

@tautschnig Removed vestigial includes and added a unit test

@hannes-steffenhagen-diffblue hannes-steffenhagen-diffblue force-pushed the feature-optional_map_lookup branch 2 times, most recently from 7f0a3c4 to 4e506bc Compare November 29, 2018 12:51
Copy link
Contributor

@allredj allredj left a comment

Choose a reason for hiding this comment

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

✔️
Passed Diffblue compatibility checks (cbmc commit: 4e506bc).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/93002859

This provides a more ergonomic interface over map::find
@hannes-steffenhagen-diffblue
Copy link
Contributor Author

CodeBuild failure due to spurious network issues, restarting CI

Copy link
Contributor

@allredj allredj left a comment

Choose a reason for hiding this comment

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

✔️
Passed Diffblue compatibility checks (cbmc commit: fa39d19).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/93043043

@hannes-steffenhagen-diffblue hannes-steffenhagen-diffblue merged commit 61b202a into diffblue:develop Nov 30, 2018
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.

5 participants