Skip to content

Conversation

@frederick-vs-ja
Copy link
Contributor

Fixes #108.

  • Replaces every occurrence of _EEN_DS in STL.natvis with a detailed expression that calculates the block size from sizeof(value_type).
  • Every _Block_size is turned into a local variable, so that it would not be instantiated eagerly.
  • No non-_Ugly identifier is introduced.
  • I think no ABI break is introduced, but I'm not very sure.

@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner January 27, 2022 03:16
@ghost
Copy link

ghost commented Jan 27, 2022

CLA assistant check
All CLA requirements met.

stl/inc/deque Outdated
_Size_type _Block = _Mycont->_Getblock(_Myoff);
_Size_type _Off = _Myoff % _Block_size;
constexpr int _Block_size = _Mydeque::_Get_block_size();
_Size_type _Block = _Mycont->_Getblock(_Myoff);
Copy link
Contributor

Choose a reason for hiding this comment

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

While you are there both could be const

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made index variables const when possible. But I'm not sure if there are some other variables expected to be made const.


using namespace std;

void test_gh_000108_push_pop_size();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for the separate declaration and definition of that function? We conventionally only define it before the main function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I partially duplicated the test.cpp file from that of Dev10_860421.

To be honest I'm not familiar with the tests of MSVC STL and don't know what are expected to be tested...

@StephanTLavavej
Copy link
Member

Thanks for looking into this, but I don't believe that we should merge this PR, even if there's a way to make the visualizers work. As mentioned in the README's Non-Goals section, we generally avoid adding non-Standard extensions, especially significant ones. The rationale is that such extensions have maintenance costs, especially as the Standard continues to evolve, and they don't help users write portable code (in fact, they can lead to users writing code that works for MSVC, only to discover that it's non-portable - there are already many ways to do that, we just try to avoid adding more).

@StephanTLavavej StephanTLavavej added decision needed We need to choose something before working on this enhancement Something can be improved labels Jan 28, 2022
@FrankHB
Copy link

FrankHB commented Jan 29, 2022

we generally avoid adding non-Standard extensions, especially significant ones

This is an QoI issue at current. Given the usefulness of the feature and the feasibility in fresh implementations, it may be proposed in some future version of the standard, so it may be potentially beneficial.

(And if it is not, to make the code portable, users who need this extension are enforced to do the work similar in this patch to replace the standard one.)

such extensions have maintenance costs, especially as the Standard continues to evolve

The changes in the patch apparently incur some costs, but the effects on the cost of long term maintenance is not definite, "especially as the Standard continues to evolve".

they don't help users write portable code

They don't help at current, until the guarantees are mandated in the future, which is at least, possible.

in fact, they can lead to users writing code that works for MSVC, only to discover that it's non-portable - there are already many ways to do that, we just try to avoid adding more

This is probably the easiest part here. The implementation can just add checks to enforce the completeness of the types by default. Then, optionally, provide a configurable macro as an extension to disable the checks, with documented guarantees and caveats on the risks of the portability.

@frederick-vs-ja
Copy link
Contributor Author

@StephanTLavavej Recently I found that libc++ has an ABI-incompatible mode (_LIBCXX_ABI_VERSION == 2) where deque is made to support incomplete element types. Once clang switches to this mode by default, it becomes practical to standardize such support.

@CaseyCarter
Copy link
Contributor

Thanks for looking into this, but I don't believe that we should merge this PR, even if there's a way to make the visualizers work. As mentioned in the README's Non-Goals section, we generally avoid adding non-Standard extensions, especially significant ones. The rationale is that such extensions have maintenance costs, especially as the Standard continues to evolve, and they don't help users write portable code (in fact, they can lead to users writing code that works for MSVC, only to discover that it's non-portable - there are already many ways to do that, we just try to avoid adding more).

Beyond our normal concern about extension behaviors, I don't want to add any more support for incomplete element types in Standard containers until WG21 solves the problem of constraining Special Member Functions for such a container. Today, vector<list<unique_ptr>> (for example) is unusable. list has a potentially-throwing move constructor, and lies about being copyable (this is what "unconstrained special member functions" means), so vector chooses to (try to) copy list<unique_ptr>s on reallocate instead of move. Since unique_ptr - and therefore list<unique_ptr> is in fact not copyable we get a compile error.

We can't constrain the special member functions without unavoidable breakage for the so-called "cyclic" incomplete element type case:

struct S {
  std::container<S> some_structs;
};

If container attempts to determine if S is copyable, the compiler must first determine if container<S> is copyable, which requires knowing if S is copyable, ad infinitum.

@StephanTLavavej
Copy link
Member

We talked about this at the weekly maintainer meeting and have consensus to close this PR. If the Standard is changed in the future to require deque<Incomplete> then merging such a change will be possible.

@StephanTLavavej StephanTLavavej added wontfix This will not be worked on and removed decision needed We need to choose something before working on this labels Feb 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Something can be improved wontfix This will not be worked on

Projects

None yet

Development

Successfully merging this pull request may close these issues.

<deque>: deque should support incomplete value types

5 participants