From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756480AbbJVICM (ORCPT ); Thu, 22 Oct 2015 04:02:12 -0400 Received: from mail-wi0-f180.google.com ([209.85.212.180]:33080 "EHLO mail-wi0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755970AbbJVICI (ORCPT ); Thu, 22 Oct 2015 04:02:08 -0400 Date: Thu, 22 Oct 2015 10:02:04 +0200 From: Ingo Molnar To: Namhyung Kim Cc: Arnaldo Carvalho de Melo , Peter Zijlstra , Jiri Olsa , LKML , David Ahern , Adrian Hunter , Borislav Petkov , Chandler Carruth , Frederic Weisbecker , Stephane Eranian , Wang Nan , Brendan Gregg Subject: Re: [PATCH 1/3] perf tools: Move callchain help messages to callchain.h Message-ID: <20151022080204.GA18916@gmail.com> References: <1445495330-25416-1-git-send-email-namhyung@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1445495330-25416-1-git-send-email-namhyung@kernel.org> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Namhyung Kim wrote: > +#define CALLCHAIN_HELP "setup and enables call-graph (stack chain/backtrace) recording: " > + > +#ifdef HAVE_DWARF_UNWIND_SUPPORT > +#define CALLCHAIN_RECORD_HELP CALLCHAIN_HELP "fp dwarf lbr" > +#else > +#define CALLCHAIN_RECORD_HELP CALLCHAIN_HELP "fp lbr" > +#endif nano-nit, could we structure such balanced #ifdefs the following way: #ifdef HAVE_DWARF_UNWIND_SUPPORT # define CALLCHAIN_RECORD_HELP CALLCHAIN_HELP "fp dwarf lbr" #else # define CALLCHAIN_RECORD_HELP CALLCHAIN_HELP "fp lbr" #endif makes the construct stand out a lot better visually. I also had another look at the help text: > output_type,min_percent[,print_limit],call_order[,branch] > +#define CALLCHAIN_REPORT_HELP "output_type (graph, flat, fractal, or none), " \ > + "min percent threshold, optional print limit, callchain order, " \ > + "key (function or address), add branches" Btw., when I first read this message in the help text yesterday, I had to read the 'min percent threshold' twice, to realize that the default 0.5 is in units of percentage - the wording wasn't entirely clear about that. Also, I had to go into the code to decode the real meaning of all the other parameters. I'd have expected them to be more obvious from reading the help text. Wording them the following way would have made things a lot more apparent to me: print_style,min_percent[,print_percent],call_order[,key] call chain tree printing style (graph|flat|fractal|none) minimum tree inclusion threshold (percent) printing threshold (percent) call chain order (caller|callee) key (function|address|branch) Note that I extended the help text with new options not mentioned in the help text but present in the current code - such as the 'branch' key. Also note that in the code I did not find any trace of the '[,branch]' and 'add branches' part present in the help text. What we have is a 'branch' option in the (optional) key parameter. I also made various edits to the help text to make it more consistent and more self-explanatory. I think we should also put the various options into a new line in the help screen, not the single line dump of text it is currently. Btw., we also have a grammar problem with all things call chains: there's 800+ occurances of 'callchain' in the perf code, and less than 20 spellings of 'call chain'. But the latter is the correct variant: Google won't even let you search for 'callchain' by default and corrects it to 'call chain' automatically. If you insist on searching for 'callchain', Google finds this number of hits: 'code callchain': 54,200 'code call chain': 141,000,000 I think it's pretty obvious what the dominant spelling is in the industry! ;-) So we should probably rename all occurances of 'callchain' to 'call chain' or 'call_chain'. Thanks, Ingo