Re: [PATCH] msvcp: Add a stub of _Concurrent_vector_base_v4.

[Piotr Caban]

Hi Hua,

the patch looks mostly good for me but I would like to ask you to make 
some changes.

On 05/05/18 16:19, Hua Meng wrote:
> +typedef struct
> +{
> - /* no virtual functions */
> - /* void* (__cdecl *allocator)(_Concurrent_vector_base_v4&, MSVCP_size_t); */
> - void *storage[3];
> - MSVCP_size_t first_block;
> - MSVCP_size_t early_size;
> - void** segment;
>
> +} _Concurrent_vector_base_v4;

The class structure size needs to match native. You can't comment 
allocator field out. Also we don't usually add comment about lack of 
virtual function, could you please remove it?
> +/* ?_Internal_assign@_Concurrent_vector_base_v4@details@Concurrency@@IAEXABV123@IP6AXPAXI@ZP6AX1PBXI@Z4@Z */
> +/* ?_Internal_assign@_Concurrent_vector_base_v4@details@Concurrency@@IEAAXAEBV123@_KP6AXPEAX1@ZP6AX2PEBX1@Z5@Z */
> +DEFINE_THISCALL_WRAPPER(_Concurrent_vector_base_v4__Internal_assign, 24)
> +void __thiscall _Concurrent_vector_base_v4__Internal_assign(_Concurrent_vector_base_v4 *this, _Concurrent_vector_base_v4 const * _Item, MSVCP_size_t len, void (__cdecl* func0)(void *, MSVCP_size_t), void (__cdecl* copy)(void *, void const *, MSVCP_size_t), void (__cdecl* func1)(void *, void const *, MSVCP_size_t))

Please avoid long lines (usually we try to fit into 80 chars per line 
but if it's not reasonable you can limit the lines to e.g. 100 chars). 
Please also be more consistent with * character placements (e.g. in 
function pointers you are sometimes adding it like "__cdecl* func" and 
sometimes like "__cdecl *func" (the second one is preferred because it's 
already used in other parts of this file, we're generally trying to 
match the surrounding code style)).

Since you don't know yet what passed functions are doing (and last 2 
function pointers have the same signature) it's hard to guess which of 
them is copy operator. You can leave this function parameter names as 
func0, func1 and func2 for now.

Please also rename _Item argument to item (probably in this case vector 
or v will make more sense).
> +MSVCP_size_t __thiscall _Concurrent_vector_base_v4__Internal_grow_to_at_least_with_result(_Concurrent_vector_base_v4 *this, MSVCP_size_t len1, MSVCP_size_t len2, void (__cdecl* copy)(void *, void const *, MSVCP_size_t), void const * e)

I guess len1 and len2 are count and element size but it can be renamed 
later. Especially taking into account that we will need to write tests 
to make sure.

The return value type should be "void*" in _Internal_compact.

Thanks,
Piotr