This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Granularizes vector.
AbandonedPublic

Authored by Mordante on Mar 10 2023, 8:43 AM.

Details

Reviewers
EricWF
Group Reviewers
Restricted Project
Summary

https://llvm.org/PR61314 reports a missing include in vector.
Unfortunately adding that include causes a circular reference.
__format/buffer.h uses vector<charT>, which is included by vector
to fix the bug.

The intention is to only let __format/buffer.h use the granularized
header, this will not use vector<bool> so it's clear which
specialization is needed. For other headers this might be possible too,
but it's not planned.

Diff Detail

Event Timeline

Mordante created this revision.Mar 10 2023, 8:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2023, 8:43 AM
Mordante updated this revision to Diff 504178.Mar 10 2023, 9:15 AM

CI fixes.

Mordante updated this revision to Diff 504349.Mar 11 2023, 3:45 AM

CI fixes.

Mordante updated this revision to Diff 504353.Mar 11 2023, 5:43 AM

CI fixes.

Mordante published this revision for review.Mar 11 2023, 7:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2023, 7:08 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
EricWF requested changes to this revision.Mar 20 2023, 12:28 PM
EricWF added a subscriber: EricWF.

I think this is a really bad idea.
Splitting the specialization out so that only some files can depend on it, while others cannot, allowing some bits of the code to only see a partial implementation is going to be bug prone.

I'm also not sure I agree with the bug. The standard only requires the declaration of the type to be available without the format include. It says nothing about requiring the type to be complete IMHO.
Like that hash<std::vector<bool>> is declared in <vector> but technically you need to include <utility> to get its definition.

This dance is done specifically so that anytime a type T with a hash specialization is seen by the user, hash<T> has been properly declared, so the compiler knows to not instantiate the default implementation.

I would like to see other alternative solutions explored. I don't think this one should proceed.

libcxx/include/__vector/vector.h
339

Where did this go?

3351

Where did this go?

This revision now requires changes to proceed.Mar 20 2023, 12:28 PM

Rereading the bug on github. I think this might deserve a standard defect report.

The spec should be a lot more clear about what exactly is provided in the library headers which provide declare specializations.

Rereading the bug on github. I think this might deserve a standard defect report.

The spec should be a lot more clear about what exactly is provided in the library headers which provide declare specializations.

I'm not sure this warrants an LWG-issue. Based on http://eel.is/c++draft/format#formatter.spec-4

If the library provides an explicit or partial specialization of formatter<T, charT>, that specialization is enabled and meets the Formatter requirements except as noted otherwise.

I think it's clear the bug report is valid.

The problem is caused by libc++ including <vector> from format. This is not something the Standard mandates.
Do you still feel there is a reason to file an LWG-issue?

I will look whether I can fix this in a different way.

Mordante abandoned this revision.Apr 18 2023, 10:25 AM
Mordante added a subscriber: ldionne.

@EricWF I've discussed this further with @ldionne. He's also not thrilled by this approach and we decided on a different approach. So I will no longer pursue this approach.

libcxx/include/module.modulemap.in