https://bugs.kde.org/show_bug.cgi?id=371916
--- Comment #18 from Julian Seward <jsew...@acm.org> --- Some comments on the v3 patch: Mostly it looks fine. Below [1] some small comments. Larger comments: * I assume there are no regressions with correctness or performance with this, yes? Could you do a self-host Memcheck run at some point? * The large size of the patch concerns me a bit. I would be happier if it could be split into two parts: (1) that creates m_xtree.c and refactors Massif to use it, but does not change the user-visible functionality at all. So it is an implementation-only change, and (2) a patch that builds on (1), that supplies the new functionality. Is that possible, if it is not a lot of work? [1] Small comments: ---------------- +typedef + struct { Addr a; const HChar* sym_name; PtrdiffT offset; } + Sym_Name_CacheEnt; I prefer to have something more descriptive than just "a" for the first field. Since it is debug-info related, it would also be good to clarify whether this is an SVMA or an AVMA or something else, per comment at the top of m_debuginfo/debuginfo.c. I suspect it's a SVMA, in which case a good name would by "sym_svma". ---------------- +" --xtree-memory=none|allocs|full profile heap memory in an xtree [none]\n" +" and produces a report at the end of the execution\n" +" none: no profiling, allocs: current allocated\n" +" size/blocks, full: profile current and cumulative\n" +" allocated size/blocks and freed size/blocks.\n" +" --xtree-memory-file=<file> xtree memory report file [xtmemory.kcg.%%p]\n" This flag only has effect for tools that replace malloc, correct? Is it listed in the section "user options for Valgrind tools that replace malloc:" ? ---------------- + This file is part of Valgrind, a dynamic binary instrumentation + framework. + + Copyright (C) 2016-2016 Philippe Waroquiers For m_xtree.c, if there is a lot of code in there which has been moved from massif and/or callgrind, and is not much changed, I think it would be diplomatic to add a line or two explaining that the original authors were Nick and/or Josef. See the top of coregrind/m_wordfm.c for an example. ---------------- // growing such a block, but for consistency (it also simplifies things) we // ignore such reallocs as well. +// XTREE??? why can't we just consider that a realloc of an ignored +// alloc is just a new alloc (i.e. do not remove the old sz from the stats). and again later. The "XTREE???" is confusing -- I don't know what it signifies. Can you maybe write instead something like "PW Nov 2016, xtree work:"? I do that in comments from time to time. ---------------- Per previous comments in the bug re CamelCase vs snake_case, I really have no problem mixing them. I like to use camelcase, with a capital first letter for type names, and snake case when function names get long. But no fixed rules. -- You are receiving this mail because: You are watching all bug changes.