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

*_allocator<void>: Make members public #586

Closed
wants to merge 1 commit into from

Conversation

roberth
Copy link
Contributor

@roberth roberth commented Nov 21, 2023

A private value_type is a problem for boost small_vector, which instantiates rebind at some point.

include/boost/container/allocator_traits.hpp:145:37: error: 'value_type' is a private member of 'traceable_allocator<void>'
   typedef typename allocator_type::value_type value_type;

[...]

gc_allocator.h:319:23: note: implicitly declared private here
  typedef void        value_type;

I don't see a reason why any of the members should be private, and arguably it violates the Liskov substitution principle. It seems that it was left private by accidental omission.

@ivmai
Copy link
Owner

ivmai commented Nov 21, 2023

Agree. But I think same should be done for gc_allocator and gc_allocator_ignore_off_page

A private value_type is a problem for boost small_vector<X>, which
instantiates rebind<void> at some point.
For example, instantiating small_vector<int, 8, traceable_allocator<int>> results in:

    include/boost/container/allocator_traits.hpp:145:37: error: 'value_type' is a private member of 'traceable_allocator<void>'
       typedef typename allocator_type::value_type value_type;

[...]

    gc_allocator.h:319:23: note: implicitly declared private here
      typedef void        value_type;

I don't see a reason why any of the members should be private, and
arguably it violates the Liskov substitution principle. It seems that
it was left private by accidental omission.
@roberth roberth force-pushed the traceable_allocator-public branch from 1efa161 to 86f63fb Compare November 22, 2023 17:32
@roberth roberth changed the title traceable_allocator<void>: Make members public *_allocator<void>: Make members public Nov 22, 2023
@roberth
Copy link
Contributor Author

roberth commented Nov 22, 2023

Good point; done.

Perhaps superfluously, I think the CI failures are unrelated (no C++ involved) and flaky (went from 4 to 2 failures for no apparent reason)

ivmai pushed a commit that referenced this pull request Nov 27, 2023
PR #586 (bdwgc).

A private value_type is a problem for boost small_vector<X>, which
instantiates rebind<void> at some point.  E.g., instantiating
small_vector<int, 8, traceable_allocator<int>> results in
"'value_type' is a private member of 'traceable_allocator<void>'"
compiler error.

I don't see a reason why any of the members should be private, and
arguably it violates the Liskov substitution principle.  It seems that
these were left private by an accidental omission.

* include/gc/gc_allocator.h (gc_allocator<void>,
gc_allocator_ignore_off_page<void>, traceable_allocator<void>): Declare
all members of the class as public.
@ivmai
Copy link
Owner

ivmai commented Nov 27, 2023

Merged

@ivmai ivmai closed this Nov 27, 2023
@roberth
Copy link
Contributor Author

roberth commented Nov 27, 2023

Thanks!

ivmai pushed a commit that referenced this pull request Dec 14, 2023
(a cherry-pick of commit 86b3bf0 from 'master')

PR #586 (bdwgc).

A private value_type is a problem for boost small_vector<X>, which
instantiates rebind<void> at some point.  E.g., instantiating
small_vector<int, 8, traceable_allocator<int>> results in
"'value_type' is a private member of 'traceable_allocator<void>'"
compiler error.

I don't see a reason why any of the members should be private, and
arguably it violates the Liskov substitution principle.  It seems that
these were left private by an accidental omission.

* include/gc_allocator.h (gc_allocator<void>,
gc_allocator_ignore_off_page<void>, traceable_allocator<void>): Declare
all members of the class as public.
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.

2 participants