https://bugs.kde.org/show_bug.cgi?id=371916
--- Comment #5 from Philippe Waroquiers <philippe.waroqui...@skynet.be> --- Thanks for the comments, here is some feedback. I will attach a new patch version, with an updated pub_tool_xtree.h. (In reply to Ivo Raisr from comment #3) > Thank you for this patch, Philippe. > I was concentrating on the interface pub_tool_xtree.h in the first run and > here are my questions and comments: > > 1. void*(*alloc_fn)(const HChar*, SizeT) in VG_(newXT)(). > What is 'const HChar *' used for? I can imagine 'SizeT' is for the size. > Perhaps if you can name the arguments, it would be clearer? Or you can give > a hint that VG_(malloc)() and VG_(free)() are typically used for alloc_fn > and free_fn? This uses the classical alloc_fn and free_fn convention, used also in other modules. See e.g. pub_tool_xarray.h or pub_tool_rangemap.h. I agree that it would be more readable to have parameter names or possibly/preferably we should define function types e.g. in pub_tool_basics.h. I have added this to my TODO list, to rework all occurences of alloc_fn/free_fn > > 2. What is 'ec' and 'IP' as referred to in VG_(newXT)()'? It is not > explained in the file header. IP is Instruction Pointer, ec is exe context. I have added some more explanation in the PURPOSE comment at the top. > > 3. What is argument 'cc' used for in VG_(newXT)()? Classical 'cost centre' for modules that are allocating memory. I have added a comment line to clarify cc is allocation cost centre. > > 4. Functions in this header file use a mixture of camelCase and > all_lower_case which is inconsistent and disturbs reader's eyes. Can you do > something with it? Humph, that is a very valuable comment, but I am not sure what style to use. Most of the current code is itself a mixture of upper case/lower case, VG_(...), ... For example, in pub_tool_hashtable.h, we find a.o. VgHashNode** VG_(HT_to_array) void VG_(HT_ResetIter) ( VgHashTable *table ); So, it seems that the convention is that there is no convention, and this new module follows the 'no convention' convention. But if we agree on a convention, I can rework this. (my preference would be to Words_Separated_By_Underscore, this is longer but IMO more readable and can be supported automatically by re-formatter tools). > > 5. I wonder why VG_(init_XtAllocs)(), VG_(add_XtAllocs)(), > VG_(sub_XtAllocs)() and VG_(img_XtAllocs)() do not use structure XtAllocs > instead of 'void *'? Am I missing something? These are functions to be used as init_data_fn/add_data_fn/sub_data_fn, so they match the argument profiles as found in VG_(newXT) > > 6. XtAllocsEvents does not need to be exported. Fixed. > > 7. For VG_(XtMemoryFull_free)(), does providing ec_alloc brings an > unnecessary burden on the tool? Perhaps m_xtree.c could cache it? In fact, the idea is that the tool is itself maintaining the alloc ec, and so the cpu/memory cost is then neglectible. If m_xtree.c would itself either maintain or (re-)compute the ec_alloc, then that would bring cpu and/or memory overhead, and the xtree interface would need to receive the allocated or block address. > > 8. VG_(XtMemory_report)() talks about xtree-memory=full, > --xtree-memory=allocs and --xtree-memory=none. How one does set these? These are the 3 values for the new command line option --xtree-memory. I have clarified the comment. > > 9. "typedef void MsFile;" Surely we can do better with implementation > hiding. Several lines above "typedef struct _XTree XTree;" was used, so > what about: > typedef struct _MsFile MsFile; In reality, MsFile is a VgFile, but I do not want to expose this, so using typedef void MsFile; was hiding it without introducing an artificial struct _MsFile. As the user cannot do much with a void, this looks to hide what it is. Not sure about what to do here. > > 10. I also wonder why callgrind and massif specific functionality is > contained in generic module xtree? Perhaps it could be located in the tools > themselves? These are implementing core functionality to output an xtree in a callgrind format or a massif format. memcheck/helgrind and massif can now each output an xtree in massif or callgrind format. So, this code cannot go into the respective tools, it must be in core. -- You are receiving this mail because: You are watching all bug changes.
