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

Add checks for extent compatability in span range constructor #181

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

libbooze
Copy link

Based on discussion in #179 / @pdimov suggestion.

few notes

  1. This will break some current code that does not exibit UB, for example constructing span of extent 3 fromboost::arrayof size 4. As this is not allowed for std::array I presumed same logic should follow for other ranges.
  2. I did not know a nicer C++11 way to produce an error message if extents do not match, so I used this mechanism with conversion of argument to resolve ambiguity
  3. regarding formatting: I found no clang-format file in repo so I did my best to do what seems right
  4. as for tests: I was trying to follow existing practices
  5. lmk if comments are too extensive
  6. if span_default_constructed_size works correctly maybe it should be moved to separate header if we think it will be useful in more places
  7. I did not add runtime checks with BOOST_ASSERT, I prefer to do that in separate PR
  8. I did not add checks for max_size, that may be useful for some fixed buffer types, e.g. static_string
  9. for std::span availability in test I used C++23 macro(I presume all C++23 compilers have all C++20 headers) since it is easier than to check for span header, if you prefer I can switch to specific span check
  10. I used boost::array as a nice range type, I know core can not depend on almost anything, but I presumed it is fine in tests a I think boost array does not depend on core, if not let me know so I make a simple test type.
  11. b2 cxxstd=03 test fails, not sure if that is fine as C++11 is new minimal Boost version

@glenfe
Copy link
Member

glenfe commented Dec 14, 2024

I'm not OK with this change. I'll provide rationale when I have some time.

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