More checks for non-compilable code, plus fix for span by beinhaerter · Pull Request #1180 · microsoft/GSL

beinhaerter · 2024-12-28T16:20:21Z

Bugfix: templated versions of span::first(), span::last() and span::subspan() now fail to compile if more elements than available are being extracted from a static sized span; previously it compiled and Expects failed during runtime
Remove some of the checks in #ifdef CONFIRM_COMPILATION_ERRORS and add compile time checks for these tests.
Acitvate some commented out checks.
Activate some deactivated checks depending on the platform
Make some death checks simpler to only check the function call that is needed to trigger the death.
clang-format

beinhaerter · 2024-12-28T16:25:18Z

I was not able to replace all checks in the touched files.

Functions where the type constraints are not checked with enable_if but with a static_assert in the function cannot be checked with the SFINAE trick. For example decltype(gsl::to_byte(char{})) x; compiles. The compilers seems to see the valid function signature, but as the function is not called, it does not compile the function body and thus ignores the static_assert(std::is_same<T, unsigned char>::value, ...);. Visibility/availability of the function does not mean compilability.

Classes where a type constraint is not checked with enable_if but with a static_assert in the class cannot be checked with the SFINAE trick. For example not_null<int> triggers the class wide static_assert(details::is_comparable_to_nullptr<T>::value, "T cannot be compared to nullptr."); even in SFINAE context.

The static_assert checks in class and function are nice for regular users as they give better warnings, but they break unit tests. How do we want to proceed with them? I am curious what your experts think about it.

tests/span_tests.cpp

beinhaerter · 2024-12-30T15:34:32Z

If desired, I can also move the static_asserts into the functions where the old #ifdef CONFIRM-COMPILATION_ERRORS were. That might be better for getting a good overview and structuring.

tests/span_tests.cpp

beinhaerter · 2025-01-01T16:43:24Z

If desired, I can also move the static_asserts into the functions where the old #ifdef CONFIRM-COMPILATION_ERRORS were. That might be better for getting a good overview and structuring.

Done.

beinhaerter · 2025-01-01T17:02:42Z

tests/notnull_tests.cpp

-    }
+    static_assert(TypeDeductionCtorCompilesFor<void*>, "TypeDeductionCtorCompilesFor<void*>");
+#if defined(_MSC_VER)
+    // Fails on gcc, clang, xcode, VS clang with


The pipeline stops at the first error. I did not check if this compiles for any of the gcc/clang/xcode versions.

That's fine. At first glance this looks like an "accepts invalid" for MSVC. We shouldn't be able to create a gsl::not_null<nullptr_t>. I will investigate further then raise the issue internally.

beinhaerter · 2025-01-01T17:25:50Z

tests/span_tests.cpp

@@ -216,6 +222,12 @@ TEST(span_test, from_pointer_length_constructor)

 TEST(span_test, from_pointer_pointer_construction)


This function has three commented out checks with comments "// this will fail the std::distance() precondition, which asserts on MSVC debug builds". I tried to activate the tests on different platforms, but then the tests failed. I could not match the comment with the death behaviour of the code.

I could not match the comment with the death behaviour of the code.

How does it fail today? I don't see a std::distance call in gsl/span, so I wonder if the comment is just out of date.

Probably also worth noting that the &arr[1] expression is UB so if compilers treat it differently, it may not be a compiler bug.

I know that two of the three EXPECT_DEATH failed because no death occured. The pipeline said line 296 and line 273 failed.

Because of ammending commits I cannot match failed pipelines to still existing commits. But I think these were the test 2 and 3, not the first one.

Tests 2 and 3 are identical? Let me know if I'm missing something. Test 1 is definitely UB.

I think we should delete test 1 and uncomment either test 2 or test 3 (and delete the other).

For std::span all three tests are UB.

This is the implementation of the gsl::span ctor.

template <std::size_t MyExtent = Extent, std::enable_if_t<MyExtent != dynamic_extent, int> = 0> constexpr explicit span(pointer firstElem, pointer lastElem) noexcept : storage_(firstElem, narrow_cast<std::size_t>(lastElem - firstElem)) { Expects(lastElem - firstElem == static_cast<difference_type>(Extent)); }

I think for GSL the relevant point is lastElem - firstElem. For the first case (&arr[0] - &arr[1]) this is OK. The length store in storage_ is not fine, but the Expects triggers.
For the checks with a nullptr the pointer subtraction should be a problem (the pointers do not point into the same object -> UB).

If I don't miss anything then test one is a test that we can activate, but tests two and three should be deleted or left commented out.

I think all tests should be deleted. It will be confusing to have tests that may start to fail with a compiler update.

If you see value in keeping them commented out for reference, that's fine. But the false comments about std::distance should be removed.

Tests 2 and 3 are identical? Let me know if I'm missing something. Test 1 is definitely UB.

Your godbolt test is range nullptr[1]..nullptr[0]. Test 1 is arr[1]..arr[0] where arr is a C style array.

I delete the tests 2 and 3. For test 1 you can decide. I think we do not need to assure this behaviour, but if we want to, it does not hurt.

carsonRadtke

Changing macro'd out code for static_asserts is a great change, thanks!

Just a couple comments/questions/suggestions. Once those are resolved, it will be ready to merge.

include/gsl/span

carsonRadtke · 2025-01-03T14:46:06Z

tests/notnull_tests.cpp

-    }
+    static_assert(TypeDeductionCtorCompilesFor<void*>, "TypeDeductionCtorCompilesFor<void*>");
+#if defined(_MSC_VER)
+    // Fails on gcc, clang, xcode, VS clang with


That's fine. At first glance this looks like an "accepts invalid" for MSVC. We shouldn't be able to create a gsl::not_null<nullptr_t>. I will investigate further then raise the issue internally.

carsonRadtke · 2025-01-03T15:03:56Z

tests/span_tests.cpp

@@ -216,6 +222,12 @@ TEST(span_test, from_pointer_length_constructor)

 TEST(span_test, from_pointer_pointer_construction)


I could not match the comment with the death behaviour of the code.

How does it fail today? I don't see a std::distance call in gsl/span, so I wonder if the comment is just out of date.

Probably also worth noting that the &arr[1] expression is UB so if compilers treat it differently, it may not be a compiler bug.

tests/span_tests.cpp

carsonRadtke · 2025-01-03T16:00:40Z

I opened #1181 to address the failing checks.

~~I will merge ASAP.~~

Edit: merged

- Remove some of the checks in `#ifdef CONFIRM_COMPILATION_ERRORS` and add compile time checks for these tests. - `algorithm_tests.cpp` and `span_test.cpp` have not been touched.

… read

plus more unit tests for these functions

carsonRadtke · 2025-01-04T17:50:15Z

Looks good! Thanks for the PR and thanks for making all the requested changes :)

beinhaerter mentioned this pull request Dec 28, 2024

strict_not_null for unique_ptr #1179

Merged

beinhaerter force-pushed the unit_test_compilation_error branch 5 times, most recently from 2c70176 to e329a72 Compare December 30, 2024 12:57

beinhaerter commented Dec 30, 2024

View reviewed changes

tests/span_tests.cpp Show resolved Hide resolved

tests/span_tests.cpp Show resolved Hide resolved

tests/span_tests.cpp Show resolved Hide resolved

beinhaerter force-pushed the unit_test_compilation_error branch 2 times, most recently from d0cd8ee to 57e5a91 Compare December 30, 2024 13:12

beinhaerter commented Dec 30, 2024

View reviewed changes

tests/span_tests.cpp Show resolved Hide resolved

beinhaerter force-pushed the unit_test_compilation_error branch 3 times, most recently from b2e7ddc to e65ece0 Compare December 30, 2024 14:23

beinhaerter commented Dec 30, 2024

View reviewed changes

tests/span_tests.cpp Outdated Show resolved Hide resolved

beinhaerter force-pushed the unit_test_compilation_error branch 3 times, most recently from 141cd59 to 466e3f7 Compare December 30, 2024 16:20

beinhaerter commented Dec 30, 2024

View reviewed changes

tests/span_tests.cpp Outdated Show resolved Hide resolved

beinhaerter force-pushed the unit_test_compilation_error branch 5 times, most recently from 711572c to a1d2c73 Compare January 1, 2025 16:42

beinhaerter force-pushed the unit_test_compilation_error branch 3 times, most recently from c855f7e to 02c02cc Compare January 1, 2025 17:00

beinhaerter commented Jan 1, 2025

View reviewed changes

beinhaerter force-pushed the unit_test_compilation_error branch from 7b8563d to 4279da5 Compare January 2, 2025 09:44

beinhaerter changed the title ~~More checks for non-compilable code~~ More checks for non-compilable code, plus fix for span Jan 2, 2025

carsonRadtke added Type: Enhancement Suggests an improvement or new feature Type: Infra Changes to build system, tests, or pipelines Status: Review Needed Needs review from GSL maintainers labels Jan 3, 2025

carsonRadtke requested changes Jan 3, 2025

View reviewed changes

Werner Henze and others added 12 commits January 3, 2025 19:08

More checks for non-compilable code

cb03a17

- Remove some of the checks in `#ifdef CONFIRM_COMPILATION_ERRORS` and add compile time checks for these tests. - `algorithm_tests.cpp` and `span_test.cpp` have not been touched.

algorithm_test, span_tests.cpp

6436dbf

non-const string::data() is available since C++17

5251973

ConversionCompilesFor

bd906e5

check_fail_on_some_platforms

d91b5c7

less fail_on_some_platforms

ac23649

check FAIL_ON_SOME_PLATFORMS

4d3c42e

Get rid of FAIL_ON_SOME_PLATFORMS

e55946d

activate commented out tests

f5ba439

move static_asserts to the original places, making the diff easier to…

b438aa9

… read

static_assert for templated span::first/last/subspan

e8fa8b1

plus more unit tests for these functions

check for dynamic_extent

e31f8d7

beinhaerter force-pushed the unit_test_compilation_error branch from 97b7c00 to e31f8d7 Compare January 3, 2025 18:09

Werner Henze added 6 commits January 3, 2025 19:10

check on which platforms the first test fails

d88c5c0

check test 2

904995f

check test 3

4d82fd8

comment out tests again

23665ed

delete tests 2 and 3

8124d30

comment test 1

e60f0ea

carsonRadtke merged commit 1cdb8d2 into microsoft:main Jan 4, 2025
83 checks passed

beinhaerter deleted the unit_test_compilation_error branch January 6, 2025 14:47

		@@ -216,6 +222,12 @@ TEST(span_test, from_pointer_length_constructor)

		TEST(span_test, from_pointer_pointer_construction)

Conversation

beinhaerter commented Dec 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

beinhaerter commented Dec 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

beinhaerter commented Dec 30, 2024

Uh oh!

Uh oh!

beinhaerter commented Jan 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carsonRadtke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

carsonRadtke commented Jan 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carsonRadtke commented Jan 4, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

beinhaerter commented Dec 28, 2024 •

edited

Loading

beinhaerter commented Dec 28, 2024 •

edited

Loading

carsonRadtke commented Jan 3, 2025 •

edited

Loading