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

Qualify _Ugly function names in standard algorithms #4004

Closed

Conversation

frederick-vs-ja
Copy link
Contributor

@frederick-vs-ja frederick-vs-ja commented Sep 2, 2023

Fixes #1596. Fixes #3842. Speculatively implements LWG-3969 to make fold_left_first_with_iter and its friends ADL-proof.

Unblocked libcxx tests:

  • std/utilities/memory/allocator.traits/allocator.traits.members/construct.pass.cpp
  • std/utilities/memory/allocator.traits/allocator.traits.members/destroy.pass.cpp
  • (partially) std/algorithms/robust_against_adl.compile.pass.cpp

Personal decisions:

  • __stdmeow internal functions (which are in the global namespace) sometimes need to be qualified. I've chosen :: instead of _CSTD because _CSTD __stdmeow looks very weird to me.
  • Not all _Ugly function names in the implementations of algorithms are qualified. If an internal function is never called with an argument that potentially triggers ADL involving a user-defined type (e.g., if it is only called with integers or vector<ptrdiff_t>), its name may be unqualified.

Discovered compiler bugs:

WIP stuffs:

  • boyer_moore_searcher and boyer_moore_horspool_searcher involve comparison of container iterators. I don't know how to fix them now.
    • Possible resolution: turn the iterator types into something like _Hash_table_iterator_provider<Args>::_Iterator (proof of concept example), which needs to break ABI.
    • A new issue opened.
  • Some ranges algorithms currently rely on structured bindings for pair and subrange which depend on ADL-found get. I'm not sure whether we should invent new return types (like ranges::meow_result) or just give up structured bindings.
    • Decision made - non-ADL-proof structured bindings are removed at this moment.
  • Investigate the bug of MSVC triggered by ranges::sort calls. It was not a bug, just my fault that missed qualification.

Thanks @miscco for the handling of raw pointers.

frederick-vs-ja and others added 2 commits September 2, 2023 12:09
Co-authored-by: Michael Schellenberger Costa <mschellenbergercosta@googlemail.com>
@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner September 2, 2023 15:09
@frederick-vs-ja frederick-vs-ja marked this pull request as draft September 2, 2023 16:05
@achabense
Copy link
Contributor

achabense commented Sep 2, 2023

I think we need to avoid ADL-based verification. For example, the new _Adl_verify_range still cannot handle the following case.
(Can we make every friend _Verify_range namespace-bound? If so, then we can replace _Adl_verify_range with namespace::_Verify_range call namespace-qualified _Verify_range in (possibly renamed)std::_Adl_verify_range.)

#include<array>

struct incomplete;
template<class T>
struct wrapper { T t; };
using inp = wrapper<incomplete>*;

int main() {
    std::array<inp, 3> arr{};
    struct inppair { inp a{}, b{}; };
    std::array<inppair, 3> arr2{};
    std::_Adl_verify_range(arr.begin(), arr.end()); // <<
    std::_Adl_verify_range(arr2.begin(), arr2.end()); // <<
}

(Also, I notice that for std::array<std::pair<inp,inp>, 3> arr{} (not inppair), even the initialization cannot be done:
image
)

@frederick-vs-ja frederick-vs-ja marked this pull request as ready for review September 2, 2023 17:05
@frederick-vs-ja
Copy link
Contributor Author

(Also, I notice that for std::array<std::pair<inp,inp>, 3> arr{} (not inppair), even the initialization cannot be done:
image
)

Thanks! I've reduced this example and added it to DevCom-10456450.

@StephanTLavavej StephanTLavavej self-assigned this Sep 6, 2023
@StephanTLavavej StephanTLavavej removed their assignment Sep 22, 2023
@StephanTLavavej

This comment was marked as resolved.

@CaseyCarter

This comment was marked as resolved.

@azure-pipelines

This comment was marked as resolved.

@StephanTLavavej
Copy link
Member

As I mentioned on Discord, the sheer magnitude of this PR makes it difficult to review. Could you refactor it either into separate PRs, or well-structured fine-grained commits (force-pushing is OK if you choose the latter)? I think I could review pure _STD additions with no clang-formatting, followed by a completely mechanical clang-format commit.

Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are real test failures after the merge. Additionally, please see my comment above - how can this PR be made more reviewable? Thanks!

@frederick-vs-ja
Copy link
Contributor Author

There are real test failures after the merge. Additionally, please see my comment above - how can this PR be made more reviewable? Thanks!

Oh, I think I should close this PR and create new, smaller PRs. Although I'm a bit unwilling to close this before creating the first new PR...

@frederick-vs-ja
Copy link
Contributor Author

Closing as new PR created.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants