Em Thu, Oct 22, 2015 at 09:13:49PM +0900, Namhyung Kim escreveu: > On Thu, Oct 22, 2015 at 5:02 PM, Ingo Molnar <mi...@kernel.org> wrote: > > > > * Namhyung Kim <namhy...@kernel.org> 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. > > OK
Done. > > > > 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. > > OK > > > > > 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. > > Did you check the man page also? I think we have (short) explanation > for each parameter and users should read it first to understand the > meaning. But I agree that the help text should also be improved to > provide quick reference. > > > > > > 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) > > Note that this 'printing threshold' is not percent. It's to limit > number of callchain entries printed for each hist entry. However it > works for --stdio only probably since it lacks interactive > collapse/expand feature. > > > > 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. > > Looking at the document, it seems branch is not a key: > > branch can be: > - branch: include last branch information in callgraph > when available. Usually more convenient to use --branch-history > for this. > > Confusingly, it was checked in parse_callchain_sort_key() but does > nothing with the sort key IIUC. > > > > > 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. > > OK These can come on a followup patch, improving the situation, right? > > > > 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'. > > Not sure about this part. Do you really think it's worth changing? > > Thanks, > Namhyung -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/