ilovepi wrote: > @ilovepi, thanks for the patience and taking out the time to give detailed > reviews. I will work on addresing your comments. One word on the tests: I was > trying to test some of the other fail conditions but seems like a lot of the > error conditions are caught by the parsing early on and do not actually > invoke our error handler (like the current existing test case). Would adding > those cases be OK regardless? Even though this patch does not directly > address those issues. In that vein, it might be better to add `clang-doc` > tests as part of a different PR, and I will be happy to work on that follow > up too. Let me know what you think.
Its no problem. If we don't mentor folks to be better contributors, we won't get new ones, and if people didn't take the time to explain the best practices to *me* in my own reviews, I wouldn't be able to do this either. As for tests if we're exercising those cases, then its fine if another mechanism triggers the error in something like the options parsing code from cl::opt. For this patch, I'd like to see a test for each of the places we added `ExitOnErr()` calls (at least to the extent that's practical). If its not trivial to do so, lets add TODOs and file Issues on GitHub as necessary, so that work doesn't get lost/forgotten. If you have a follow up patch, then you can remove the TODOs as you address them. https://github.com/llvm/llvm-project/pull/141699 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
