TileDB icon indicating copy to clipboard operation
TileDB copied to clipboard

Remove serialization non C.41 constructors from fragment_metadata_from_capnp

Open teo-tsirpanis opened this issue 6 months ago • 9 comments

SC-40131


TYPE: NO_HISTORY DESC: Refactor the FragmentMetadata class to reduce mutable state.

teo-tsirpanis avatar Jan 29 '24 22:01 teo-tsirpanis

@teo-tsirpanis let's not work more on this PR. I started to review and it became obvious to me we need to design something different here. The 30 parameters constructor doesn't look good.

KiterLuc avatar Jan 31 '24 13:01 KiterLuc

Undrafting, regardless of whether we will take it, it is finished and ready for review.

teo-tsirpanis avatar Jan 31 '24 17:01 teo-tsirpanis

I restored some move constructors and am getting errors like static_assert failed: 'T must be constructible from either (allocator_arg_t, const Alloc&, Types...) or (Types..., const Alloc&) if uses_allocator_v<remove_cv_t<T>, Alloc> is true'. Any idea on how to fix them?

teo-tsirpanis avatar Apr 19 '24 13:04 teo-tsirpanis

I restored some move constructors and am getting errors like static_assert failed: 'T must be constructible from either (allocator_arg_t, const Alloc&, Types...) or (Types..., const Alloc&) if uses_allocator_v<remove_cv_t<T>, Alloc> is true'. Any idea on how to fix them?

@davisp any ideas here?

KiterLuc avatar Apr 19 '24 17:04 KiterLuc

That’s a rule for constructors on PMR containers stored inside other pmr containers. Either the constructor has to have the last argument for the Allocator, or the first argument must be the special trait marker and the second argument is the allocator argument. So likely this was adding the move constructor without providing an argument to accept the allocator.

davisp avatar Apr 29 '24 14:04 davisp

Pablo Halpern covers the container constructor stuff during this part of his cppcon talk: https://youtu.be/v3dz-AKOVL8?t=2360&si=qKn94l2h8x9uwtHT

davisp avatar Apr 29 '24 14:04 davisp

Thanks for the help! The problem was in d4d9007b293ccbecdf160de3e63a6b57a749cb60, and after a couple more modifications it builds locally.

teo-tsirpanis avatar Apr 29 '24 19:04 teo-tsirpanis

CI is green. @KiterLuc can we merge it?

teo-tsirpanis avatar May 13 '24 18:05 teo-tsirpanis