Re: profile mode: statistics for vector and map

[Gedare Bloom <gedare at rtems dot org>]

Updated patch attached to address the comments. I was not able to
figure out how to build the libstdc++ documentation to verify my doc
changes although I did run xmllint successfully.

-Gedare

On Thu, May 9, 2013 at 4:41 PM, François Dumont <frs.dumont@gmail.com> wrote:
> On 05/08/2013 12:57 AM, Gedare Bloom wrote:
>>
>> On Tue, May 7, 2013 at 6:16 PM, Gedare Bloom <gedare@rtems.org> wrote:
>>>
>>> On Tue, May 7, 2013 at 4:56 PM, François Dumont <frs.dumont@gmail.com>
>>> wrote:
>>>>
>>>>      IMHO we should adopt the same kind of design I have introduced for
>>>> the
>>>> unordered containers that is to say a base class containing in its
>>>> constructor the call to the instance registration and in the destructor
>>>> the
>>>> unregistration. This way special functions could be defaulted which will
>>>> simplify maintenance and we would never forget the registration like in
>>>> the
>>>> vector constructor from initializer_list<value_type> one.
>>>>
>>> That's fine. If I have time I'll study your approach in more detail. I
>>> had a look at the patch you sent, and your approach seems more robust.
>>> The mechanisms here with macros and hand-inserted function call-outs
>>> to wrap the "real" operations is easy to mess up.
>>>
>> If someone can rework the existing vector code for the
>> profiler_vector_size.h/profiler_container_size.h files using the
>> approach suggested, then I can probably generalize the approach to the
>> container statistics that I implemented without too much trouble. I
>> used the vector size as the basis for my design and implementation.
>
>
> We can deal with this later for sure. I might do so after what I am working
> on at the moment.
>
> Regarding the patch itself I took a closer look:
> - I saw a FIXME comment, can it be fixed or removed before going in ? I also
> saw a XXX one, can it just be removed or explained ?
> - I have been surprised by the *_statistics_invalid_operator macros. It
> doesn't seem to be used anymore and I can't imagine what is an invalid
> operator in the profile mode so it should be removed.
> - Sometimes code could be simplified like in:
>
> +        size_type rv;
>          __profcxx_map_to_unordered_map_find(this, size());
> -        return _Base::count(__x);
> +        __profcxx_map_statistics_find(this, size()); // FIXME
> +        rv = _Base::count(__x);
> +        return rv;
>
> The temporary rv local variable doesn't seem to have any interest.
> - There is a 80 columns limit, it looks like sometimes the lines you have
> added exceeded this limit.
> - We might need some documentation about the new statistics. Profile mode is
> well documented, check doc/xml/manual/profile_mode.xml for the place to add
> your doc.
>
> Did you complete what is necessary to commit this kind of patch yourself ? I
> won't be able to give you the ok to commit it but people that can will ask
> you to have signed the necessary papers.
>
> François
>

Attachment: 0001-profile-mode-Add-Statistics-for-vector-and-map.patch
Description: Binary data