<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;"><br><div><div>On May 4, 2013, at 4:07 PM, Dan 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;">Sorry Andrew I didn't read your e-mail correctly the first time. I've<br>added some primitive tests to "unittests/Support/CommandLineTest.cpp" in<br>each patch.<br><br>I have also modified the first patch so that empty option categories are<br>not printed in the output of -help, they are however printed in the<br>output of -help-hidden.<br><br>The modified patches are attached.<br><br>Something to note is I discovered a problem with one of the tests<br>(CommandLineTest.ParseEnvironmentToLocalVar) in "CommandLineTest.cpp".<br>This test creates a command line option on the stack. This option is<br>subsequently destroyed on leaving the test but the command line system<br>still has a pointer to that option. If an attempt is made to access<br>registered options (e.g. through ParseCommandLineOptions() or<br>getRegisteredOptions() ) after this test the program will SEGFAULT.<br><br>I worked around this by having my tests that need to access registered<br>options rule run before the test<br>CommandLineTest.ParseEnvironmentToLocalVar.<br><br>This could be fixed in general by implementing destructors for the<br>Option objects that unregisters them from the command line system but<br>this probably adds unnecessary overhead as the options are intended to<br>be declared statically anyway.<br></div></blockquote><div><br></div><div>Awesome. Committed with no modifications as r181253-4.</div><div><br></div><div>Thanks</div><div>-Andy</div></div><div><br><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;">On 04/05/13 12:47, Dan Liew wrote:<br><blockquote type="cite">See my responses below.<br>On 03/05/13 22:39, Andrew Trick wrote:<br><blockquote type="cite">Daniel,<br><br>I reviewed your most recent patch (MOREFIXES) and your<br>getRegisteredOptions patch. They both look very nice.<br><br>I have one suggestion. For normal (non-hidden) help, we may have empty<br>categories, which looks odd. Can you just suppress printing the category<br>in that case?<br></blockquote><br>Yes I can do that. Would you like empty category printing to be<br>suppressed for -help-hidden as well or should we show it?<br><br><blockquote type="cite">Also, would you mind adding a couple simple tests to CommandLineTest.cpp?<br></blockquote><br>I have not used LLVM's testing infrastructure very much so I'm going to<br>need some guidance here.<br><br>I looked at the LLVM testing infrastructure documentation and it<br>mentioned the regression tests (obviously not what I want) and<br>test-suite (sounded promising as documentation said it built whole<br>programs). I took a look at the test-suite but it didn't look<br>appropriate as I could not see a way an obvious way to link to the<br>LLVMSupport library.<br><br>I also took a look at the unittests in the LLVM repository and found<br>some existing in "unittests/Support/CommandLineTest.cpp".<br><br>Is it here that you would like me to add tests?<br><br>If so do you have any suggestions on testing the OptionCategory class? I<br>can test whether or not an option has a category set but there isn't a<br>way (AFAIK) to test the output of --help (which is the whole point of<br>the feature) against some known output in the gtest framework.<br><br><blockquote type="cite">Otherwise, it's ready to commit. Will you need me to check it in?<br></blockquote>Y<br>Yes I will need you to check it in as I do not have commit access.<br><br>Thanks,<br>Dan.<br>_______________________________________________<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><br><br></blockquote><span><0001-ATFIXES-Support-command-line-option-categories.patch></span><span><0002-ATFIXES-Implemented-public-interface-for-modifying-registere.patch></span></div></blockquote></div><br></body></html>