This is an archive of the discontinued LLVM Phabricator instance.

[libc++] static_assert preconditions in vector
Needs RevisionPublic

Authored by philnik on Aug 17 2022, 12:02 PM.

Details

Reviewers
ldionne
Mordante
var-const
EricWF
Group Reviewers
Restricted Project
Summary

This improves the diagnostics when instantiating member functions with types that don't meet the preconditions. For example, this error message now gets produced when instantiating resize() with a type that's not copy constructible:

In file included from erros.cpp:1:
/home/nikolas/llvm-projects/libcxx/build/include/c++/v1/vector:1886:5: error: static assertion failed due to requirement '__is_cpp17_copy_insertable<std::allocator<NotCopyInsertable>>::value': value_type has to be Cpp17CopyInsertable
    static_assert(__is_cpp17_copy_insertable<_Allocator>::value, "value_type has to be Cpp17CopyInsertable");
    ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
erros.cpp:10:5: note: in instantiation of member function 'std::vector<NotCopyInsertable>::resize' requested here
  v.resize(1, NotCopyInsertable(1));
    ^
In file included from erros.cpp:1:
In file included from /home/nikolas/llvm-projects/libcxx/build/include/c++/v1/vector:299:
In file included from /home/nikolas/llvm-projects/libcxx/build/include/c++/v1/__split_buffer:21:
/home/nikolas/llvm-projects/libcxx/build/include/c++/v1/__memory/allocator.h:165:28: error: call to implicitly-deleted copy constructor of 'NotCopyInsertable'
        ::new ((void*)__p) _Up(_VSTD::forward<_Args>(__args)...);
                           ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/nikolas/llvm-projects/libcxx/build/include/c++/v1/__memory/allocator_traits.h:292:13: note: in instantiation of function template specialization 'std::allocator<NotCopyInsertable>::construct<NotCopyInsertable, const NotCopyInsertable &>' requested here
        __a.construct(__p, _VSTD::forward<_Args>(__args)...);
            ^
/home/nikolas/llvm-projects/libcxx/build/include/c++/v1/vector:1000:25: note: in instantiation of function template specialization 'std::allocator_traits<std::allocator<NotCopyInsertable>>::construct<NotCopyInsertable, const NotCopyInsertable &, void>' requested here
        __alloc_traits::construct(this->__alloc(), std::__to_address(__pos), __x);
                        ^
/home/nikolas/llvm-projects/libcxx/build/include/c++/v1/vector:1043:15: note: in instantiation of member function 'std::vector<NotCopyInsertable>::__construct_at_end' requested here
        this->__construct_at_end(__n, __x);
              ^
/home/nikolas/llvm-projects/libcxx/build/include/c++/v1/vector:1889:15: note: in instantiation of member function 'std::vector<NotCopyInsertable>::__append' requested here
        this->__append(__sz - __cs, __x);
              ^
erros.cpp:10:5: note: in instantiation of member function 'std::vector<NotCopyInsertable>::resize' requested here
  v.resize(1, NotCopyInsertable(1));
    ^
erros.cpp:5:3: note: copy constructor is implicitly deleted because 'NotCopyInsertable' has a user-declared move constructor
  NotCopyInsertable(NotCopyInsertable&&);
  ^
2 errors generated.

Clangd emits the first error with this patch and the second error without.

Diff Detail

Event Timeline

philnik created this revision.Aug 17 2022, 12:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2022, 12:02 PM
philnik requested review of this revision.Aug 17 2022, 12:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2022, 12:02 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne requested changes to this revision.Sep 15 2022, 9:24 AM
ldionne added inline comments.
libcxx/include/__memory/allocator_traits.h
370–371

Let's add a comment like:

// Implement named requirements from http://eel.is/c++draft/container.alloc.reqmts

And then, while we're at it, we should also implement Cpp17EmplaceConstructible and other named requirements, and probably check them in the places where they are mandated.

371–392

Observation: our previous definition was wrong, since it looked for a Allocator.construct() method only, and the Cpp17FooInsertable requirements are based on allocator_traits<Alloc>::construct. Your implementation does it the right way. Can you please add a test that would fail if we did not do it correctly? This should be possible by creating an allocator A without a .construct() method, but defining allocator_traits<A>::construct (or, in C++<something>, allocator_traits::construct might even be defined for you automatically, although the test should work in older standards too so maybe don't rely on that).

423

For LLVM 15, I would be tempted to go and remove the static assert in __uninitialized_allocator_move_if_noexcept since it is wrong (due to checking Allocator.construct() instead of allocator_traits::construct:

static_assert(__is_cpp17_move_insertable<_Alloc>::value,
                "The specified type does not meet the requirements of Cpp17MoveInsertable");

We can also fix the trait, however I don't think this is a major issue and I would not spend a lot of time trying to backport a complicated fix. Can you ping me when you have a patch on top of LLVM 15? I can cherry-pick it.

libcxx/test/libcxx/containers/sequences/vector/preconditions.verify.cpp
10

This is kind of light on the details.

This revision now requires changes to proceed.Sep 15 2022, 9:24 AM
rymiel added a subscriber: rymiel.Sep 15 2022, 10:56 AM
rymiel added inline comments.
libcxx/include/vector
391

Just happened to notice in passing: There seems to be 6 little typos in total which say "Inserable"

philnik updated this revision to Diff 470795.Oct 26 2022, 6:00 AM
philnik marked 4 inline comments as done.
  • Address comments
libcxx/include/__memory/allocator_traits.h
371–392

AFAICT construct was always optional, so I think it's OK to just use an allocator without construct.

libcxx/test/libcxx/containers/sequences/vector/preconditions.verify.cpp
10

A single section doesn't apply here, and I don't think it makes sense to list the subsections of [vector]. Saying it's in [vector] also seems quite obvious.

ldionne accepted this revision.Oct 27 2022, 9:11 AM
ldionne added inline comments.
libcxx/include/__memory/allocator_traits.h
383
This revision is now accepted and ready to land.Oct 27 2022, 9:11 AM
ldionne requested changes to this revision.Oct 27 2022, 9:13 AM

Actually, I'd like to see what the impact of this change is (given that our own tests seem to be violating some of these requirements). We should do it, but I still want to see the impact before we land it, because that will inform our communication about it. Can you ping me once CI is passing and I'll try this on our internal code base as a guinea pig.

This revision now requires changes to proceed.Oct 27 2022, 9:13 AM
EricWF added a subscriber: EricWF.Oct 27 2022, 11:16 AM

Please write out a more descriptive explanation of the benifit and rational for this change. It would be nice to have a lot more than a single line attached to the eventual commit.

Additionally, I assume the only thing this does is make the diagnostics for already ill-formed code slightly easier to grok? If so, we should consider whether that improvement is worth the additional compile time cost and maintenance cost.
Perhaps by providing concrete examples of where the existing diagnostics are grossly insufficient or need improving.

libcxx/test/libcxx/containers/sequences/vector/preconditions.verify.cpp
30

Can you add an annotation that actually shows that the line below is triggering any diagnostic at all? Something like // expected-note {{from here}}

philnik updated this revision to Diff 472787.Nov 2 2022, 3:13 PM
philnik marked 2 inline comments as done.

Address comments

Actually, I'd like to see what the impact of this change is (given that our own tests seem to be violating some of these requirements). We should do it, but I still want to see the impact before we land it, because that will inform our communication about it. Can you ping me once CI is passing and I'll try this on our internal code base as a guinea pig.

There was actually a bug in __is_cpp17_emplace_constructible. I've added a regression test.

philnik edited the summary of this revision. (Show Details)Nov 2 2022, 3:29 PM
EricWF added a comment.Nov 2 2022, 6:14 PM

This change doesn't improve conformance, right? It just attempts to make the diagnostics nicer in a handful of cases where the error occurs in the immediate function call to the allocator?

libcxx/include/__memory/allocator_traits.h
374

Can't a user provided overload of operator, sneak in here?

403–414

Is _And still lazy? If so, do we really need the additional instantiations

libcxx/test/libcxx/containers/sequences/vector/preconditions.verify.cpp
58

expected-note

EricWF added a comment.Nov 2 2022, 6:16 PM

I'm not convinced your average user knows what copy insertable means, and I think the static assert is more opaque than the templated messages, which while noiser, contain more information.

It would be interesting to actually do a user study to find out what non-expert users think.

EricWF requested changes to this revision.Nov 2 2022, 6:21 PM

I would like other people to weigh in on this patch. I don't think these changes pull their weight. I'm not convinced they add clarity. And I'm certain they add complexity and cost -- also if we get the assert wrong (and I don't think we will), but we risk breaking totally valid code that would otherwise compile.

I feel like the conformance test is "does the function compile" and nothing more is needed.

Change my mind.

libcxx/include/__memory/allocator_traits.h
398

Why are these not template aliases? Wouldn't that have a smaller compile time effect?

This revision now requires changes to proceed.Nov 2 2022, 6:21 PM
EricWF added a comment.Nov 2 2022, 6:24 PM

Sorry for the repeated comments. But if I understand the description correctly, the diagnostic below is no longer emitted?

error: call to implicitly-deleted copy constructor of 'NotCopyInsertable'
        ::new ((void*)__p) _Up(_VSTD::forward<_Args>(__args)...);

Because that's what I would need to see to understand *how to fix* the issue.

If we're blocking the user from seeing that, this patch should not land.