[clang-tools-extra] [clang-doc] Refactor error handling to use ExitOnError (PR #141699)
Paul Kirth via cfe-commits
cfe-commits at lists.llvm.org
Thu May 29 10:36:05 PDT 2025
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
More information about the cfe-commits
mailing list