Dev/annagrin/remove explicit not null constructor #743
Conversation
|
@neilmacintosh could you please review? |
|
|
||
| { | ||
| int t = 42; | ||
| int* p = &t; |
There was a problem hiding this comment.
Am I right in thinking we are missing tests for actual implicit construction of not_null from things like explicit pointer variables and address-of operations? Something like:
helper(p);
It seems that's the whole thing people need this change for, so we'd want to ensure it works. Highly possible I missed it somewhere though. I suppose you could argue helper(&t); is equivalent, but it would be nice to see the specific syntax just to be sure something hasn't been missed.
There was a problem hiding this comment.
yeah, you are right, the idea was that &i cover those tests, but having them added would be nice. Will change.
| strict_not_null<int*> snn = &x; | ||
| strict_helper(&x); | ||
| strict_helper_const(&x); | ||
| strict_helper(return_pointer()); |
There was a problem hiding this comment.
It would be nice to have the equivalents of these back in the not_null tests. That symmetry would help show the difference between the two very clearly (I'm thinking particularly of the return_pointer type tests here).
tests/test.cpp
Outdated
| @@ -20,7 +20,7 @@ | |||
|
|
|||
| // blanket turn off warnings from CppCoreCheck from catch | |||
| // so people aren't annoyed by them when running the tool. | |||
| #pragma warning(disable : 26401 26409 26415 26418 26426 26429 26432 26433 26434 26435 26436 26439 26440 26443 26444 26446 26447 26451 26460 26461 26466 26472 26481 26482 26485 26492 26493 26494 26495 26496 26497) // from catch | |||
| #pragma warning(disable : 26401 26409 26415 26418 26426 26429 26432 26433 26434 26435 26436 26439 26440 26443 26444 26446 26447 26451 26455 26456 26460 26461 26466 26472 26481 26482 26485 26492 26493 26494 26495 26496 26497) // from catch | |||
There was a problem hiding this comment.
It might be simpler to just blanket-disable ALL /analyze warnings from Catch?
There was a problem hiding this comment.
yes, i don't think we care about warnings in test.cpp itself... will change,
| @@ -72,13 +72,13 @@ public: | |||
| static_assert(std::is_assignable<T&, std::nullptr_t>::value, "T cannot be assigned nullptr."); | |||
|
|
|||
| template <typename U, typename = std::enable_if_t<std::is_convertible<U, T>::value>> | |||
There was a problem hiding this comment.
I don't understand how this ctor doesn't get selected and allow nullptr_t through (which is explicitly excluded by the next ctor below). Maybe it all works for whatever obscure reason I can't remember?
There was a problem hiding this comment.
deleted constructor from not_null seems to take precedence. Note that commenting out deleted constructor does compile. My efforts to find explanation in the standard were futile, maybe someone else can help?
There was a problem hiding this comment.
= delete doesn't mean that the pertinent function doesn't participate in overload resolution, it means the program is ill-formed if the function is chosen by overload resolution.
EDIT: and in this particular case, the deleted std::nullptr_t constructor is selected over the U&& constructor template by the "non-templates beat templates with equivalent conversion sequences" tie breaker in http://eel.is/c++draft/over.match.best#1.6.
There was a problem hiding this comment.
@CaseyCarter thanks, this exactly what I was searching for!
…not_null We added explicit not_null constructor in version 2.0.0. It proved very difficult to switch to the new version for large code bases that adopted previous versions of gsl, due to not_null used extensively in the code. Still, using explicit constructor is very benefitial for new code, since it encorages better API design and make null checks intentional. To resolve the issue, this change: - removes explicit keyword from not_null constructor - removes unneded sloppy_not_null type - adds strict_not_null type to behave the same way as v2 not_null - updates tests
…not_null We added explicit not_null constructor in version 2.0.0. It proved very difficult to switch to the new version for large code bases that adopted previous versions of gsl, due to not_null used extensively in the code. Still, using explicit constructor is very benefitial for new code, since it encorages better API design and make null checks intentional. To resolve the issue, this change: - removes explicit keyword from not_null constructor - removes unneded sloppy_not_null type - adds strict_not_null type to behave the same way as v2 not_null - updates tests
annagrin
force-pushed
the
dev/annagrin/remove_explicit_not_null_constructor
branch
from
7450292
to
a82f1e7
Compare
|
thanks @neilmacintosh and @CaseyCarter for reviews! |
Removed explicit
not_nullconstructor,sloppy_not_null, addedstrict_not_nullWe added explicit
not_nullconstructor in version 2.0.0.It proved very difficult to switch to the new version for
large code bases that adopted previous versions of GSL,
due to
not_nullused extensively in the code. Still, usingexplicit constructor is very beneficial for new code, since
it encourages better API design and make null checks intentional.
To resolve the issue, this change:
not_nullconstructorsloppy_not_nulltypestrict_not_nulltype to behave the same way as v2not_null- updates teststhis closes #699