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

Bug fixing #1151

Open
wants to merge 69 commits into
base: dmitrykobets-msft/experiment/fix-span-2020-gtest-failures
Choose a base branch
from

Conversation

CEEANPROJEC
Copy link

dmitrykobets-msft and others added 30 commits December 9, 2021 14:54
* Fix C++20 bugs and tests
* Rework CI for C++2020 tests
* Update readme compiler versions
Bump clang and Xcode supported versions and add support for VS with LLVM
[SF.12: Prefer the quoted form of #include for files relative to the including file and the angle bracket form everywhere else](https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rs-incform)

Additionally changed #include order in `span` so that all `span_ext` is in the GSL include block and not in the STL include block.

Fixes issues microsoft#1025.

Co-authored-by: Werner Henze <w.henze@avm.de>
The following reserved identifiers are being used specifically to target certain MSVC constructs, so suppress the warning in VS 2022 (LLVM) "... is reserved because it starts with '_' followed by a capital letter":
- _Unchecked_type
- _Verify_range
- _Verify_offset
- _Unwrapped
- _Unwrap_when_unverified
- _Seek_to
- _Unchecked_begin
- _Unchecked_end
…osoft#1043)

In the implementation of gsl::narrow, there is a comparison `static_cast<U>(t) != u` which may be comparing two floats.
The comparison here is done purposefully to categorize ill effects of narrowing conversion, since the values being compared *should* be the same when compared with `operator==`. 
Note, using #pragma GCC will suppress this warning for both GCC and Clang.
This file uses std::exception, so it should include the appropriate header.

Normally it gets the STL's <exception> header included via the gsl/assert file, but this is skipped with _HAS_EXCEPTIONS=0. I understand _HAS_EXCEPTIONS is undocumented and unsupported, but regardless, the appropriate header should be included here.

Alternatively, gsl/narrow should be modified to support _HAS_EXCEPTIONS=0, like gsl/assert was. But I'm not proposing that change. <exception> does define std::exception even with _HAS_EXCEPTIONS=0.
As per the CoreGuidelines, gsl::narrow is defined in terms of static_cast.
Suppress es.46, which suggests converting the static_cast into gsl::narrow
Both checks for Expects(ExtentType::size() != dynamic_extent); in storage_type are always useless. storage_type<ExtentType> is only ever created with ExtentType == extent_type<Extent>, where Extent has type std::size_t and is the extent of the span.

Looking at extent_type<std::size_t Ext>::size():

- if Ext != dynamic_extent, then size() always returns Ext, and therefore size() != dynamic_extent
- if Ext == dynamic_extent, then size() returns extent_type<dynamic_extent>::size_. size_ can only be set via one of two constructors:
  - constexpr explicit extent_type(size_type size), which already does the check in question
  - constexpr explicit extent_type(extent_type<Other> ext) : size_(ext.size()), which simply relies on the other extent's size() method
So there is no way for ExtentType::size() == dynamic_extent.
Co-authored-by: microsoft-github-policy-service[bot] <77245923+microsoft-github-policy-service[bot]@users.noreply.github.com>
* Test solution

* Mark dynamic_extent as inline, compiler-version-permitting
actions/runner-images#5930 recently updated the NDK version, resulting in test breakages. Update the version.
GCC 4.8.0 - 7.0 has a bug (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56480) involving specialization in a namespace enclosing the specialized template. This PR fixes an appearance of this bug in the span header.
This PR future-proofs the fix made in microsoft#1053, which is now outdated due to further updates to the NDK version in the Azure VMs used as part of the CI.
These warnings were found by running clang-tidy 15.
Somewhere along the way, GSL's implementation of final_act and finally seems to have become way overthought. This PR is to re-simplify these facilities back to what C++ Core Guidelines C.30 said which is simple and clear and works. It just copies the invocable thing, and doesn't bother trying to optimize the copy. This should be fine, because we're typically passing something that's cheap to copy, often a stateless lambda.

The problem in microsoft#846 appears to be because finally looks like was originally written as a const&/&& overload (its state at the time that issue was opened)... to eliminate a copy when you invoke it with a temporary. If so, then the && was probably never intended to be a forwarder, but an rvalue reference that tripped over the horrid C++ syntax collision where a && parameter magically instead means a forwarding reference because the type happens to be a template parameter type here. So I suspect the original author was just trying to write an rvalue overload, and the forwarder that's there now was never intended at all.
Closes issue microsoft#550, which highlighted overhead in not_null::get for larger types such as shared_ptr. Every call to get would return a copy of the contained value.
This PR implements Herb's suggestion for changing the return type of not_null::get. The not_null's value will now only be copied if it is "trivially copyable"; otherwise, it will be returned by const reference.
Note: this change also forces the returned copy to be const.
- Move all install logic inside gsl_install.cmake
  - This makes reading the logic easier, and avoids the enable language
    issue with `GNUInstallDirs` by having it included after the project
    call
- Have all functions inside gsl_functions.cmake
- Use CMake idiom PROJECT_IS_TOP_LEVEL
- Update README.md
This avoids propagating -std=c++14 as a compile option

Currently if you use less than CMake 3.8 and you install the project
`-std=c++14` gets added to the INTERFACE_COMPILE_OPTIONS. This forces
users to use C++ 14 or remove the property from the imported target.

The solution is to raise the minimum and use `cxx_std_14`
The Clang compiler for MSVC in Visual Studio 2022 17.4.33122.133 used by the test runner has a bug which raises -Wdeprecated "out-of-line definition of constexpr static data member is redundant in C++17 and is deprecated" in gsl/span, when compiling with C++14. Temporarily suppress this warning for MSVC Clang with C++14.
Guidelines issue 2006 removes the null check inside not_null::get, since the contained pointer is already guaranteed to be not-null upon construction.

Resolves microsoft#1051
NaN != Nan, so the comparisons used in gsl::narrow will always throw when attempting to cast a NaN value. This may be surprising, so document it.
This solves issue microsoft#1070 and removes the class `string_span`. The only content remaining in the header file `gsl/string_span` is the `*zstring` types.
This also removes the `string_span_tests.cpp` unit tests as these were only for the deprecated `string_span` class.

Co-authored-by: Werner Henze <werner.henze+gitcommits@posteo.de>
beinhaerter and others added 28 commits February 21, 2023 17:11
The `#i13-do-not-pass-an-array-as-a-single-pointer` anchor seems to be auto generated based on the title of the section. The `#Ri-array` anchor is explicitely written in the source, so this should be a better link.
`<gsl/span>` is intended to mirror `<span>` in the STL. `<span_ext>` provides convenience features that aren't part of `<span>`.
Address microsoft#1095
- Capitalize inital letter (make both tables consistent)
- Add links to docs/headers.md
- Latest cmake requirement is 3.14
- Mark text as code
Suppress "warning C26481: Don't use pointer arithmetic. Use span instead (bounds.1)." in the code that impements `span`.
`size_bytes()` returns the span's size in bytes. 
Assuming the span was constructed with an accurate size parameter, the check `size() < dynamic_extent / sizeof(element_type)` isn't required, since `size_t(-1)` (which is `dynamic_extent`) represents the size of the address space, so the number of bytes will never exceed it and in practice won't even come close.
Otherwise, it is not actually feasible to detect cases when the size parameter does not correspond to the dimensions of the underlying data pointer. In these cases, the relationship `size() < dynamic_extent / sizeof(element_type)` is simply one of many ways in which the `size()` could be incorrect, and serves no necessary purpose.

Resolves microsoft#1012
…sons with `not_null` (microsoft#1106)

Using `<`,`<=`,`>`,`>=` to compare unrelated pointers gives an unspecified result according to the standard.
This PR replaces the usage of these operators in `gsl::not_null` with the STL counterparts, which would leverage any implementation-defined strict total ordering for pointers.

Resolves microsoft#880
Turns out supporting GSL.natvis perfectly is quite difficult.

There is no universal way to consume .natvis files that would satisfy everyone.

I thought the solution was to use the /NATVIS linker option. But it turns out that actually embeds information into the PDB. So I'm not sure how to properly support the Ninja generator...

That's not even accounting for the fact target_link_options doesn't play nicely with /NATVIS

When you just add the file via target_sources the visual studio solution will just pick it up and recognize it without adding it to the PDB file. Which won't affect the binary and is what most devs want.

This all comes down to the fact that /NATVIS files have native integration with visual studio that makes it difficult to use with non-visual studio solutions. /NATVIS almost works but embeds itself into the PDB which not everyone wants, and not everyone generates PDBs either.

Docs for natvis files and /NATVIS
- https://learn.microsoft.com/en-us/visualstudio/debugger/create-custom-views-of-native-objects?view=vs-2022
- https://learn.microsoft.com/en-us/cpp/build/reference/natvis-add-natvis-to-pdb?view=msvc-170

So my current solution is to just simplify the existing CMake code, and install the natvis so the user can decide.

closes microsoft#1084
These overloads don't seem to be in a usable state, and their original purpose is no longer clear. Deprecate them.
Resolves microsoft#1092
Headers that were previously prefixed with `gsl_` were renamed to drop the `gsl_` prefix in microsoft#946, and the original version deprecated.
The deprecation happened a long time ago, so it is now time to remove these headers entirely.
…soft#1125)

With `string_span` having been deprecated (microsoft#931, microsoft#945) and removed (microsoft#1074), the header `<gsl/string_span>` now only contains the definitions for the `zstring` family. Update the name accordingly from `<gsl/string_span>` to `<gsl/zstring>`. The old header is now deprecated and should no longer be used and will be removed in some future release.
Without this change a `gsl::not_null<class_type>` triggers these `noexcept` warnings:
```
.../gsl/include/gsl/pointers:162:50: warning: noexcept-expression evaluates to ‘false’ because of a call to ‘constexpr gsl::details::value_or_reference_return_t<T> gsl::not_null<T>::get() const [with T = class_type*; gsl::details::value_or_reference_return_t<T> = class_type* const]’ [-Wnoexcept]
  162 |                 const not_null<U>& rhs) noexcept(noexcept(lhs.get() == rhs.get()))
      |                                                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
.../gsl/include/gsl/pointers:119:55: note: but ‘constexpr gsl::details::value_or_reference_return_t<T> gsl::not_null<T>::get() const [with T = class_type*; gsl::details::value_or_reference_return_t<T> = class_type* const]’ does not throw; perhaps it should be declared ‘noexcept’
  119 |     constexpr details::value_or_reference_return_t<T> get() const
      |                                                       ^~~
```

Co-authored-by: Werner Henze <w.henze@avm.de>
The header file uses `std::declval`, so it needs to `#include <utility>`.
Before my PR microsoft#1122 `gsl/pointers` was gcc 8.4 compatible. Now it is not. This commit makes it compatible with gcc 8.4 again.
This macro is a relic of the old implementation of GSL's header. It is unused and can be removed.
- Add anchor for `finally` in `headers.md` so that the link in `README.md` can work
- In `README.md` add code formatting for the character types of the `zstring` and `string_span` types
- In `README.md` change code formatting to links for GSL types
- Vertical alignment
Two warnings were being emitted in the MSVC+LLVM tests. 

The warning `-Wunsafe-buffer-usage` is initially introduced in some capacity here https://reviews.llvm.org/D137346 pointing to documentation at https://discourse.llvm.org/t/rfc-c-buffer-hardening/65734. The warning is a stylistic checker whose goal is to "emit a warning every time an unsafe operation is performed on a raw pointer". This type of programming model is not useful for library implementations of types such as `span`, where direct manipulation of raw pointers is inevitable, so disable the warning altogether.

There is also a false-positive warning llvm/llvm-project#65689 that I've disabled inline.
…ved with no exception (microsoft#1135)

This enables possible optimisations for trivial types. This also avoids a bug
in std::variant::emplace from GNU's libstdc++.

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106547
This fixes annotations on task runs like the following:

    The following actions uses node12 which is deprecated and will be forced
    to run on node16: actions/checkout@v2. For more info:
    https://github.blog/changelog/2023-06-13-github-actions-all-actions-will-run-on-node16-instead-of-node12-by-default/
* Suppress unsafe-buffer-usage
Co-authored-by: Werner Henze <w.henze@avm.de>
Copy link
Contributor

@CEEANPROJEC please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"
Contributor License Agreement

Contribution License Agreement

This Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
and conveys certain license rights to Microsoft Corporation and its affiliates (“Microsoft”) for Your
contributions to Microsoft open source projects. This Agreement is effective as of the latest signature
date below.

  1. Definitions.
    “Code” means the computer software code, whether in human-readable or machine-executable form,
    that is delivered by You to Microsoft under this Agreement.
    “Project” means any of the projects owned or managed by Microsoft and offered under a license
    approved by the Open Source Initiative (www.opensource.org).
    “Submit” is the act of uploading, submitting, transmitting, or distributing code or other content to any
    Project, including but not limited to communication on electronic mailing lists, source code control
    systems, and issue tracking systems that are managed by, or on behalf of, the Project for the purpose of
    discussing and improving that Project, but excluding communication that is conspicuously marked or
    otherwise designated in writing by You as “Not a Submission.”
    “Submission” means the Code and any other copyrightable material Submitted by You, including any
    associated comments and documentation.
  2. Your Submission. You must agree to the terms of this Agreement before making a Submission to any
    Project. This Agreement covers any and all Submissions that You, now or in the future (except as
    described in Section 4 below), Submit to any Project.
  3. Originality of Work. You represent that each of Your Submissions is entirely Your original work.
    Should You wish to Submit materials that are not Your original work, You may Submit them separately
    to the Project if You (a) retain all copyright and license information that was in the materials as You
    received them, (b) in the description accompanying Your Submission, include the phrase “Submission
    containing materials of a third party:” followed by the names of the third party and any licenses or other
    restrictions of which You are aware, and (c) follow any other instructions in the Project’s written
    guidelines concerning Submissions.
  4. Your Employer. References to “employer” in this Agreement include Your employer or anyone else
    for whom You are acting in making Your Submission, e.g. as a contractor, vendor, or agent. If Your
    Submission is made in the course of Your work for an employer or Your employer has intellectual
    property rights in Your Submission by contract or applicable law, You must secure permission from Your
    employer to make the Submission before signing this Agreement. In that case, the term “You” in this
    Agreement will refer to You and the employer collectively. If You change employers in the future and
    desire to Submit additional Submissions for the new employer, then You agree to sign a new Agreement
    and secure permission from the new employer before Submitting those Submissions.
  5. Licenses.
  • Copyright License. You grant Microsoft, and those who receive the Submission directly or
    indirectly from Microsoft, a perpetual, worldwide, non-exclusive, royalty-free, irrevocable license in the
    Submission to reproduce, prepare derivative works of, publicly display, publicly perform, and distribute
    the Submission and such derivative works, and to sublicense any or all of the foregoing rights to third
    parties.
  • Patent License. You grant Microsoft, and those who receive the Submission directly or
    indirectly from Microsoft, a perpetual, worldwide, non-exclusive, royalty-free, irrevocable license under
    Your patent claims that are necessarily infringed by the Submission or the combination of the
    Submission with the Project to which it was Submitted to make, have made, use, offer to sell, sell and
    import or otherwise dispose of the Submission alone or with the Project.
  • Other Rights Reserved. Each party reserves all rights not expressly granted in this Agreement.
    No additional licenses or rights whatsoever (including, without limitation, any implied licenses) are
    granted by implication, exhaustion, estoppel or otherwise.
  1. Representations and Warranties. You represent that You are legally entitled to grant the above
    licenses. You represent that each of Your Submissions is entirely Your original work (except as You may
    have disclosed under Section 3). You represent that You have secured permission from Your employer to
    make the Submission in cases where Your Submission is made in the course of Your work for Your
    employer or Your employer has intellectual property rights in Your Submission by contract or applicable
    law. If You are signing this Agreement on behalf of Your employer, You represent and warrant that You
    have the necessary authority to bind the listed employer to the obligations contained in this Agreement.
    You are not expected to provide support for Your Submission, unless You choose to do so. UNLESS
    REQUIRED BY APPLICABLE LAW OR AGREED TO IN WRITING, AND EXCEPT FOR THE WARRANTIES
    EXPRESSLY STATED IN SECTIONS 3, 4, AND 6, THE SUBMISSION PROVIDED UNDER THIS AGREEMENT IS
    PROVIDED WITHOUT WARRANTY OF ANY KIND, INCLUDING, BUT NOT LIMITED TO, ANY WARRANTY OF
    NONINFRINGEMENT, MERCHANTABILITY, OR FITNESS FOR A PARTICULAR PURPOSE.
  2. Notice to Microsoft. You agree to notify Microsoft in writing of any facts or circumstances of which
    You later become aware that would make Your representations in this Agreement inaccurate in any
    respect.
  3. Information about Submissions. You agree that contributions to Projects and information about
    contributions may be maintained indefinitely and disclosed publicly, including Your name and other
    information that You submit with Your Submission.
  4. Governing Law/Jurisdiction. This Agreement is governed by the laws of the State of Washington, and
    the parties consent to exclusive jurisdiction and venue in the federal courts sitting in King County,
    Washington, unless no federal subject matter jurisdiction exists, in which case the parties consent to
    exclusive jurisdiction and venue in the Superior Court of King County, Washington. The parties waive all
    defenses of lack of personal jurisdiction and forum non-conveniens.
  5. Entire Agreement/Assignment. This Agreement is the entire agreement between the parties, and
    supersedes any and all prior agreements, understandings or communications, written or oral, between
    the parties relating to the subject matter hereof. This Agreement may be assigned by Microsoft.

@CEEANPROJEC
Copy link
Author

@CEEANPROJEC please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{ceean project}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service i agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

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.

None yet