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

using std::optional in favor of bool return #29

Closed
wants to merge 2 commits into from
Closed

using std::optional in favor of bool return #29

wants to merge 2 commits into from

Conversation

sbrkopac
Copy link
Contributor

@sbrkopac sbrkopac commented Jan 7, 2019

Pull Request Template

Description

Utilizing std::optional rather than a bool check for returning a user value. This allows the library to transform code from:

int value;
if(object->getValue("foo", value))
{
}

Into its c++17 equivalent:

auto value = object->getValue("foo");
if(value)
{
}

Type of change

  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@sbrkopac sbrkopac changed the title using std::optional in favor bool return using std::optional in favor of bool return Jan 7, 2019
@sbrkopac
Copy link
Contributor Author

sbrkopac commented Jan 7, 2019

I also tagged in a fix for a typo. Let me know if I need to open a new PR for that.

@openscenegraph
Copy link

This won't work. I had to remove std::optionall use elsewhere due to portability issues.

@sbrkopac
Copy link
Contributor Author

sbrkopac commented Jan 7, 2019

Portability issues from where?

@vsg-dev
Copy link
Owner

vsg-dev commented Jan 7, 2019

I think @tomhog found issues with std::optional on either the Android port. The line of least resistance was simply to replace the std::optional usage.

Could you generate a PR for the typo fix. Thanks.

@vsg-dev
Copy link
Owner

vsg-dev commented Jan 9, 2019

I'm closing this PR as it risks cross platform build. I have applied the typo fix:

5d05f3f

@vsg-dev vsg-dev closed this Jan 9, 2019
@sbrkopac
Copy link
Contributor Author

sbrkopac commented Jan 9, 2019

I know it's difficult with the speed at which you are prototyping, but is it possible to have a blurb somewhere about the different c++17 features that can't be used due to X reason?

@vsg-dev
Copy link
Owner

vsg-dev commented Jan 10, 2019

I have wondered about writing a C++ usage document, from the perspective of what bits of C++17 etc. are used where, but adding what parts don't currently work well across platforms would be a good addition too. I can't promise this right away but I'll continue thinking about it.

@tomhog
Copy link
Contributor

tomhog commented Jan 10, 2019

Hi @sbrkopac

So the issue at the moment is on macOS std::optional and std::any are currently in the std::experimental namespace (I'm assuming it'll be the same for iOS). So for now we've avoided them to prevent the code requiring conditional compilation in lots of places.

Hopefully apply move these into the standard std namespace in the near future.

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.

4 participants