<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 7, 2013, at 2:51 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;">Thanks Andrew.<br><br>Looks like the patches will make it into the 3.3 release :)<br><br>If you think the added features are worth noting in the the Release<br>notes for 3.3 then please apply the attached patch (provided you're<br>happy with the wording).<br></div></blockquote><div><br></div>Looks good. r181331.</div><div>-Andy</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;"><br>Thanks,<br>Dan Liew.<br><br>On 06/05/13 23:03, Andrew Trick wrote:<br><blockquote type="cite"><br>On May 4, 2013, at 4:07 PM, Dan Liew <<a href="mailto:daniel.liew@imperial.ac.uk">daniel.liew@imperial.ac.uk</a><br><<a href="mailto:daniel.liew@imperial.ac.uk">mailto:daniel.liew@imperial.ac.uk</a>>> wrote:<br><br><blockquote type="cite">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></blockquote><br>Awesome. Committed with no modifications as r181253-4.<br><br>Thanks<br>-Andy<br><br><blockquote type="cite">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<br>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><span class="Apple-converted-space"> </span><<a href="mailto:llvm-commits@cs.uiuc.edu">mailto: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><0001-ATFIXES-Support-command-line-option-categories.patch><0002-ATFIXES-Implemented-public-interface-for-modifying-registere.patch><br></blockquote><br></blockquote><br><span><0001-Add-two-points-to-release-notes-about-recent-command.patch></span></div></blockquote></div><br></body></html>