Opened 5 years ago

Closed 4 years ago

#15534 closed task (fixed)

HaikuDepot: Lists in the Model use Custom List Class

Reported by: apl-haiku Owned by: stippi
Priority: high Milestone: R1/beta3
Component: Applications/HaikuDepot Version: R1/Development
Keywords: List BObjectList Cc:
Blocked By: Blocking:
Platform: All

Description

Some of the data-model in HaikuDepot (HD) uses a custom list class List<T>. There is a warning in this class because it is using memmove on C++ objects which is not allowed;

../haiku/src/apps/haikudepot/textview/ParagraphLayout.h:121:36:   required from here
../haiku/src/apps/haikudepot/List.h:273:11: warning: 'void* memcpy(void*, const void*, size_t)' writing to an object of type 'class TextSpan' with no trivial copy-assignment; use copy-assignment or copy-initialization instead [-Wclass-memaccess]
     memcpy(fItems, other.fItems, fCount * sizeof(ItemType));

The List has a notion of being able to maintain a sort ordering in order to speed-up searches into the list.

It is possible to stop the warning from failing the build with the following line in the Jamfile;

ObjectC++Flags Model.cpp  : -Wno-error=class-memaccess ;

One this problem is resolved; remove this line from the Jamfile if it is there.

However perhaps a better solution would be to use BObjectList or std::list.

Change History (3)

comment:1 by apl-haiku, 5 years ago

Some discussion in the PR here. Waddlesplash says regarding the short-term fix proposed;

I'd rather we not silence the warning entirely, and instead add some sort of assert to confirm that classes are trivially movable, at least on GCC 4+.

...

use std::is_trivially_move_constructible on C++11 in a static_assert() block to verify the class is indeed trivially movable. Then the memmove() will be correct.

...

Still it would probably be better to swap-out the collection classes used.

comment:2 by apl-haiku, 4 years ago

Trying this out...

...
class List {
	static_assert(std::is_trivially_move_constructible<ItemType>::value,
		"the list class is only able to be used with trivially move "
		"constructible types");

...yields...

../haiku/src/apps/haikudepot/model/PackageInfo.h:394:17:   required from here
../haiku/src/apps/haikudepot/List.h:23:16: error: static assertion failed: the list class is only able to be used with trivially move constructible types

...so I guess those memmove and memcpy operations might not be entirely safe.

comment:3 by apl-haiku, 4 years ago

Milestone: UnscheduledR1/beta3
Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.