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

Dev/annagrin/sloppy not null #712

Merged
merged 12 commits into from
Aug 13, 2018

Conversation

annagrin
Copy link

@annagrin annagrin commented Aug 2, 2018

Created a helper class to help move code that uses implicit not_null constructor to explicit. The transition would work as follows:

  • replace all occurrences of not_null in your code by sloppy_not_null
  • compile - compilation should be successful
  • replace some sloppy_not_nulls by not_null, fix compilation errors,
    redesign as needed, compile and test
  • repeat until no sloppy_not_nulls remain

The transition to explicit not_null constructor is worth it:

  • it minimized checking for null
    Including cases when not_null is silently transformed into a raw pointer and back along the call chain
  • it makes API and developer intentions cleaner
    It propagates not_null types on API to their correct place, removes assertions and simplifies code
  • it follows C++ Core Guidelines
    not_null should be a stricter type than a raw pointer, so implicit conversion is hiding useful information and potential failures

In the near future, tooling should be created to help insert explicit constructor calls and make the transition easier.

This is partially resolves issue #699 (the rest needs to be done in tooling).

@neilmacintosh
Copy link
Collaborator

@annagrin I like the idea of offering this as a concrete and temporary transition step in the absence of tooling. This class is really just to help people migrate from earlier versions of not_null to the current version. I like that it is clearly isolated off in its own header. I would suggest we could go further and place it somewhere like a samples directory or something similar. I certainly think it shouldn't live in the 'gsl' directory. I don't think it should be automatically distributed with the other headers that make up the GSL or mistaken as part of the library.

@annagrin annagrin force-pushed the dev/annagrin/sloppy_not_null branch from bd92692 to e8fc0b8 Compare August 13, 2018 05:40
@annagrin
Copy link
Author

Done, thanks!

@annagrin annagrin merged commit 6241b3f into microsoft:master Aug 13, 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.

2 participants