<html><head><meta http-equiv="Content-Type" content="text/html charset=iso-8859-1"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div>Daniel,</div><div><br></div><div>I reviewed your most recent patch (MOREFIXES) and your getRegisteredOptions patch. They both look very nice.</div><div><br></div><div>I have one suggestion. For normal (non-hidden) help, we may have empty categories, which looks odd. Can you just suppress printing the category in that case?</div><div><br></div><div>Also, would you mind adding a couple simple tests to CommandLineTest.cpp?</div><div><br></div><div>Otherwise, it's ready to commit. Will you need me to check it in?</div><div><br></div><div>-Andy</div><br><div><div>On May 3, 2013, at 6:58 AM, Daniel Liew <<a href="mailto:daniel.liew@imperial.ac.uk">daniel.liew@imperial.ac.uk</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">I've tried to address the issues you mentioned Tobias. The patch is<br>attached.<br><br>Have you been able to look at the patch for the second feature (<br>cl::getRegisteredOptions() function ) I want to add?<br><br>Thanks,<br>Dan.<br><br>On 03/05/13 13:36, Tobias Grosser wrote:<br><blockquote type="cite">On 05/02/2013 10:14 PM, Daniel Liew wrote:<br><blockquote type="cite">Oh dear there was a mistake in the first patch I gave you. I forgot to<br>remove comments relating to -help-cat and -help-cat-hidden (which was<br>part my old implementation)<br><br>I've attached the amended patch and also the patch for<br>cl::getRegisteredOptions() feature (now has documentation as part of the<br>patch and line endings cleaned up)<br><br>Sorry for the confusion.<br></blockquote><br>Hi Daniel,<br><br>thanks for addressing my comments. The new patch looks already very<br>nice. I tested it and it continues to work as expected. I have a last<br>set of minor stylistic comments, but the patch seems to be OK after<br>those have been addressed. Maybe you can repost the patch with the last<br>fixes, such that Andrew can have another look.<br><br>Thanks,<br>Tobias<br><br><blockquote type="cite">+Grouping options into categories<br>+--------------------------------<br>+<br>+If our program has a large number of options it may become difficult<br>for users<br>+of your tool to navigate the output of ``-help``. To alleviate this<br>problem we<br></blockquote>... of our tool ...<br><br>Either address the reader using 'you' or 'us'. Mixing those forms in a<br>single sentence seems confusing.<br><br><blockquote type="cite">+can declare option categories by statically declaring a<br>`cl::OptionCategory`_<br>+and then place our options into these categories using the `cl::cat`_<br>option<br>+attribute.<br>+<br>+.. code-block:: c++<br>+<br>+ cl::OptionCategory StageSelectionCat("Stage Selection Options",<br>+ "These control which stages<br>are run.");<br>+<br>+ cl::opt<bool> preprocessor("E",cl::desc("Run preprocessor stage."),<br>+ cl::cat(StageSelectionCat));<br>+<br>+ cl::opt<bool> noLink("c",cl::desc("Run all stages except linking."),<br>+ cl::cat(StageSelectionCat));<br>+<br></blockquote><br>I would call the previous options PreProcessor and NoLink according to:<br><a href="http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly">http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly</a><br><br><br><blockquote type="cite">Option Modifiers<br>----------------<br><br>@@ -1385,6 +1438,27 @@ For example:<br><br> cl::extrahelp("\nADDITIONAL HELP:\n\n This is the extra help\n");<br><br>+.. _cl::OptionCategory:<br>+<br>+The ``cl::OptionCategory`` class<br>+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^<br>+<br>+The ``cl::OptionCategory`` class is a simple class for declaring<br>+Option categories.<br>+<br>+.. code-block:: c++<br>+<br>+ namespace cl {<br>+ class OptionCategory;<br>+ }<br>+<br>+A option category must have a name and optionally a description which<br>are passed<br>+to the constructor as ``const char*``.<br>+<br>+Note that declaring an option category before parsing options(e.g.<br>statically)<br></blockquote> ^ space?<br><br>I would add a space between options and '('.<br><br><blockquote type="cite">//===----------------------------------------------------------------------===//<br><br>// Basic, shared command line option processing machinery.<br>@@ -528,6 +540,7 @@ static void ExpandResponseFiles(unsigned argc,<br>const char*const* argv,<br> }<br>}<br><br>+<br></blockquote><br>You add an unnecessary newline here.<br><br><blockquote type="cite">static cl::opt<bool><br>PrintOptions("print-options",<br>@@ -1306,6 +1447,24 @@ PrintAllOptions("print-all-options",<br> cl::desc("Print all option values after command line<br>parsing"),<br> cl::Hidden, cl::init(false));<br><br>+void HelpPrinterWrapper::operator=(bool Value) {<br>+ if (Value == false)<br>+ return;<br>+<br>+ // Decide which printer to invoke. If more than one option category is<br>+ // registered then it is useful to show the categorized help<br>instead of<br>+ // uncategorized help.<br>+ if (RegisteredOptionCategories->size() > 1) {<br>+ // Using categorized printer so should unhide -help-list so user<br>can have<br></blockquote> ^^^^ This 'so' seems useless and a<br>little confusing.<br><br>Cheers,<br>Tobi<br></blockquote><br><span><0001-MOREFIXES-Support-command-line-option-categories.patch></span>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a></div></blockquote></div><br></body></html>