feedc0de/GSL
1
0
Fork 0
forked from microsoft/GSL
Code Pull Requests Activity

Dev/annagrin/remove explicit not null constructor (#743)

Browse Source 
* Added c++17 test configurations for clang5.0 and clang6.0

* Removed explicit not_null constructor, sloppy_not_null, added strict_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

* Removed explicit not_null constructor, sloppy_not_null, added strict_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

* fixed build break for gcc7

* added more tests

* added more non-compiling tests

* Addressed PR comments and suppressed a code analysis warning

* Fixed test failure in not_null tests
This commit is contained in:
Anna Gringauze
2019-01-14 16:45:47 -08:00
committed by GitHub
parent 7a7d025ffa
commit 9ff6e19ea9
8 changed files with 384 additions and 291 deletions
Download Patch File Download Diff File Expand all files Collapse all files
tests/CMakeLists.txt
+1 -1
View File
@@ -107,7 +107,7 @@ add_gsl_test(utils_tests)
add_gsl_test(owner_tests)
add_gsl_test(byte_tests)
add_gsl_test(algorithm_tests)
add_gsl_test(sloppy_notnull_tests)
add_gsl_test(strict_notnull_tests)
# No exception tests
tests/notnull_tests.cpp
+90 -32
View File
@@ -25,7 +25,7 @@
#include <catch/catch.hpp> // for AssertionHandler, StringRef, CHECK, TEST_...
#include <gsl/pointers> // for not_null, operator<, operator<=, operator>
#include <gsl/pointers> // for not_null, operator<, operator<=, operator>
#include <algorithm> // for addressof
#include <memory> // for shared_ptr, make_shared, operator<, opera...
@@ -128,47 +128,105 @@ struct NonCopyableNonMovable
};
GSL_SUPPRESS(f.4) // NO-FORMAT: attribute
bool helper(not_null<int*> p)
{
return *p == 12;
}
bool helper(not_null<int*> p) { return *p == 12; }
GSL_SUPPRESS(f.4) // NO-FORMAT: attribute
bool helper_const(not_null<const int*> p)
{
return *p == 12;
}
bool helper_const(not_null<const int*> p) { return *p == 12; }
int* return_pointer() { return nullptr; }
const int* return_pointer_const() { return nullptr; }
GSL_SUPPRESS(con.4) // NO-FORMAT: attribute
TEST_CASE("TestNotNullConstructors")
{
{
#ifdef CONFIRM_COMPILATION_ERRORS
not_null<int*> p = nullptr; // yay...does not compile!
not_null<std::vector<char>*> p = 0; // yay...does not compile!
not_null<int*> p; // yay...does not compile!
std::unique_ptr<int> up = std::make_unique<int>(120);
not_null<int*> p = up;
not_null<int*> p = nullptr; // yay...does not compile!
not_null<std::vector<char>*> p1 = 0; // yay...does not compile!
not_null<int*> p2; // yay...does not compile!
std::unique_ptr<int> up = std::make_unique<int>(120);
not_null<int*> p3 = up;
// Forbid non-nullptr assignable types
not_null<std::vector<int>> f(std::vector<int>{1});
not_null<int> z(10);
not_null<std::vector<int>> y({1, 2});
// Forbid non-nullptr assignable types
not_null<std::vector<int>> f(std::vector<int>{1});
not_null<int> z(10);
not_null<std::vector<int>> y({1, 2});
#endif
int i = 12;
auto rp = RefCounted<int>(&i);
not_null<int*> p(rp);
CHECK(p.get() == &i);
}
not_null<std::shared_ptr<int>> x(
std::make_shared<int>(10)); // shared_ptr<int> is nullptr assignable
{
// from shared pointer
int i = 12;
auto rp = RefCounted<int>(&i);
not_null<int*> p(rp);
CHECK(p.get() == &i);
#ifdef GSL_THROW_ON_CONTRACT_VIOLATION
int* pi = nullptr;
CHECK_THROWS_AS(not_null<decltype(pi)>(pi), fail_fast);
#endif
not_null<std::shared_ptr<int>> x(
std::make_shared<int>(10)); // shared_ptr<int> is nullptr assignable
int* pi = nullptr;
CHECK_THROWS_AS(not_null<decltype(pi)>(pi), fail_fast);
}
{
// from pointer to local
int t = 42;
not_null<int*> x = &t;
helper(&t);
helper_const(&t);
CHECK(*x == 42);
}
{
// from raw pointer
// from not_null pointer
int t = 42;
int* p = &t;
not_null<int*> x = p;
helper(p);
helper_const(p);
helper(x);
helper_const(x);
CHECK(*x == 42);
}
{
// from raw const pointer
// from not_null const pointer
int t = 42;
const int* cp = &t;
not_null<const int*> x = cp;
helper_const(cp);
helper_const(x);
CHECK(*x == 42);
}
{
// from not_null const pointer, using auto
int t = 42;
const int* cp = &t;
auto x = not_null<const int*>{cp};
CHECK(*x == 42);
}
{
// from returned pointer
CHECK_THROWS_AS(helper(return_pointer()), fail_fast);
CHECK_THROWS_AS(helper_const(return_pointer()), fail_fast);
}
}
template<typename T>
template <typename T>
GSL_SUPPRESS(con.4) // NO-FORMAT: attribute
void ostream_helper(T v)
{
@@ -474,5 +532,5 @@ TEST_CASE("TestMakeNotNull")
#endif
}
static_assert(std::is_nothrow_move_constructible<not_null<void *>>::value, "not_null must be no-throw move constructible");
static_assert(std::is_nothrow_move_constructible<not_null<void*>>::value,
"not_null must be no-throw move constructible");
tests/sloppy_notnull_tests.cpp
-124
View File
@@ -1,124 +0,0 @@
///////////////////////////////////////////////////////////////////////////////
//
// Copyright (c) 2015 Microsoft Corporation. All rights reserved.
//
// This code is licensed under the MIT License (MIT).
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
// THE SOFTWARE.
//
///////////////////////////////////////////////////////////////////////////////
#ifdef _MSC_VER
// blanket turn off warnings from CppCoreCheck from catch
// so people aren't annoyed by them when running the tool.
#pragma warning(disable : 26440 26426) // from catch
// Fix VS2015 build breaks in Release
#pragma warning(disable : 4702) // unreachable code
#endif
#include <catch/catch.hpp> // for AssertionHandler, StringRef, CHECK, TEST_...
#include <gsl/pointers> // for not_null, operator<, operator<=, operator>
#include <samples/gsl_transition> // for sloppy_not_null
namespace gsl
{
struct fail_fast;
} // namespace gsl
using namespace gsl;
using namespace gsl_helpers;
GSL_SUPPRESS(f.4) // NO-FORMAT: attribute
bool helper(not_null<int*> p) { return *p == 12; }
GSL_SUPPRESS(f.4) // NO-FORMAT: attribute
bool helper_const(not_null<const int*> p) { return *p == 12; }
GSL_SUPPRESS(f.4) // NO-FORMAT: attribute
bool sloppy_helper(sloppy_not_null<int*> p) { return *p == 12; }
GSL_SUPPRESS(f.4) // NO-FORMAT: attribute
bool sloppy_helper_const(sloppy_not_null<const int*> p) { return *p == 12; }
TEST_CASE("TestSloppyNotNull")
{
{
// raw ptr <-> sloppy_not_null
int x = 42;
const sloppy_not_null<int*> snn = &x;
sloppy_helper(&x);
sloppy_helper_const(&x);
CHECK(*snn == 42);
}
{
// sloppy_not_null -> sloppy_not_null
int x = 42;
sloppy_not_null<int*> snn1{&x};
const sloppy_not_null<int*> snn2{&x};
sloppy_helper(snn1);
sloppy_helper_const(snn1);
CHECK(snn1 == snn2);
}
{
// sloppy_not_null -> not_null
int x = 42;
sloppy_not_null<int*> snn{&x};
const not_null<int*> nn1 = snn;
const not_null<int*> nn2{snn};
helper(snn);
helper_const(snn);
CHECK(snn == nn1);
CHECK(snn == nn2);
}
{
// not_null -> sloppy_not_null
int x = 42;
not_null<int*> nn{&x};
const sloppy_not_null<int*> snn1{nn};
const sloppy_not_null<int*> snn2 = nn;
sloppy_helper(nn);
sloppy_helper_const(nn);
CHECK(snn1 == nn);
CHECK(snn2 == nn);
std::hash<sloppy_not_null<int*>> hash_snn;
std::hash<not_null<int*>> hash_nn;
CHECK(hash_nn(snn1) == hash_nn(nn));
CHECK(hash_snn(snn1) == hash_nn(nn));
CHECK(hash_nn(snn1) == hash_nn(snn2));
CHECK(hash_snn(snn1) == hash_snn(nn));
}
#ifdef CONFIRM_COMPILATION_ERRORS
{
sloppy_not_null<int*> p{nullptr};
}
#endif
}
static_assert(std::is_nothrow_move_constructible<sloppy_not_null<void*>>::value,
"sloppy_not_null must be no-throw move constructible");
tests/strict_notnull_tests.cpp
+193
View File
@@ -0,0 +1,193 @@
///////////////////////////////////////////////////////////////////////////////
//
// Copyright (c) 2015 Microsoft Corporation. All rights reserved.
//
// This code is licensed under the MIT License (MIT).
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
// THE SOFTWARE.
//
///////////////////////////////////////////////////////////////////////////////
#ifdef _MSC_VER
// blanket turn off warnings from CppCoreCheck from catch
// so people aren't annoyed by them when running the tool.
#pragma warning(disable : 26440 26426) // from catch
// Fix VS2015 build breaks in Release
#pragma warning(disable : 4702) // unreachable code
#endif
#include <catch/catch.hpp> // for AssertionHandler, StringRef, CHECK, TEST_...
#include <gsl/pointers> // for not_null, operator<, operator<=, operator>
namespace gsl
{
struct fail_fast;
} // namespace gsl
using namespace gsl;
GSL_SUPPRESS(f.4) // NO-FORMAT: attribute
bool helper(not_null<int*> p) { return *p == 12; }
GSL_SUPPRESS(f.4) // NO-FORMAT: attribute
bool helper_const(not_null<const int*> p) { return *p == 12; }
GSL_SUPPRESS(f.4) // NO-FORMAT: attribute
bool strict_helper(strict_not_null<int*> p) { return *p == 12; }
GSL_SUPPRESS(f.4) // NO-FORMAT: attribute
bool strict_helper_const(strict_not_null<const int*> p) { return *p == 12; }
int* return_pointer() { return nullptr; }
const int* return_pointer_const() { return nullptr; }
GSL_SUPPRESS(con.4) // NO-FORMAT: attribute
TEST_CASE("TestStrictNotNull")
{
{
// raw ptr <-> strict_not_null
int x = 42;
#ifdef CONFIRM_COMPILATION_ERRORS
strict_not_null<int*> snn = &x;
strict_helper(&x);
strict_helper_const(&x);
strict_helper(return_pointer());
strict_helper_const(return_pointer_const());
#endif
const strict_not_null<int*> snn1{&x};
helper(snn1);
helper_const(snn1);
CHECK(*snn1 == 42);
}
{
// strict_not_null -> strict_not_null
int x = 42;
strict_not_null<int*> snn1{&x};
const strict_not_null<int*> snn2{&x};
strict_helper(snn1);
strict_helper_const(snn1);
strict_helper_const(snn2);
CHECK(snn1 == snn2);
}
{
// strict_not_null -> not_null
int x = 42;
strict_not_null<int*> snn{&x};
const not_null<int*> nn1 = snn;
const not_null<int*> nn2{snn};
helper(snn);
helper_const(snn);
CHECK(snn == nn1);
CHECK(snn == nn2);
}
{
// not_null -> strict_not_null
int x = 42;
not_null<int*> nn{&x};
const strict_not_null<int*> snn1{nn};
const strict_not_null<int*> snn2{nn};
strict_helper(nn);
strict_helper_const(nn);
CHECK(snn1 == nn);
CHECK(snn2 == nn);
std::hash<strict_not_null<int*>> hash_snn;
std::hash<not_null<int*>> hash_nn;
CHECK(hash_nn(snn1) == hash_nn(nn));
CHECK(hash_snn(snn1) == hash_nn(nn));
CHECK(hash_nn(snn1) == hash_nn(snn2));
CHECK(hash_snn(snn1) == hash_snn(nn));
}
#ifdef CONFIRM_COMPILATION_ERRORS
{
strict_not_null<int*> p{nullptr};
}
#endif
}
#if defined(__cplusplus) && (__cplusplus >= 201703L)
GSL_SUPPRESS(con.4) // NO-FORMAT: attribute
TEST_CASE("TestStrictNotNullConstructorTypeDeduction")
{
{
int i = 42;
strict_not_null x{&i};
helper(strict_not_null{&i});
helper_const(strict_not_null{&i});
CHECK(*x == 42);
}
{
int i = 42;
int* p = &i;
strict_not_null x{p};
helper(strict_not_null{p});
helper_const(strict_not_null{p});
CHECK(*x == 42);
}
{
auto workaround_macro = []() {
int* p1 = nullptr;
const strict_not_null x{p1};
};
CHECK_THROWS_AS(workaround_macro(), fail_fast);
}
{
auto workaround_macro = []() {
const int* p1 = nullptr;
const strict_not_null x{p1};
};
CHECK_THROWS_AS(workaround_macro(), fail_fast);
}
{
int* p = nullptr;
CHECK_THROWS_AS(helper(strict_not_null{p}), fail_fast);
CHECK_THROWS_AS(helper_const(strict_not_null{p}), fail_fast);
}
#ifdef CONFIRM_COMPILATION_ERRORS
{
strict_not_null x{nullptr};
helper(strict_not_null{nullptr});
helper_const(strict_not_null{nullptr});
}
#endif
}
#endif // #if defined(__cplusplus) && (__cplusplus >= 201703L)
static_assert(std::is_nothrow_move_constructible<strict_not_null<void*>>::value,
"strict_not_null must be no-throw move constructible");
tests/test.cpp
+2 -1
View File
@@ -20,7 +20,8 @@
// 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
#include <CodeAnalysis/Warnings.h>
#pragma warning(disable : ALL_CODE_ANALYSIS_WARNINGS) // from catch
#endif // _MSC_VER
#include <catch/catch.hpp>