tejohnson added a comment. In D91410#2401751 <https://reviews.llvm.org/D91410#2401751>, @OikawaKirie wrote:
> In D91410#2400018 <https://reviews.llvm.org/D91410#2400018>, @tejohnson wrote: > >> > > > >> ... the new checking is a mix of assert and fatal errors. Is that intended? > > No. The added checks are based on the checks on other calls to the > `Target::createXXX` functions in this file or other related files. If there > are any fatal errors nearby (e.g. llvm/lib/LTO/ThinLTOCodeGenerator.cpp:581 > vs 569), the check will be a fatal error; and if there are any assertions > (e.g. llvm/lib/CodeGen/LLVMTargetMachine.cpp:43,45,52 vs 60) or the calls are > never checked (e.g. llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:300), the > added check will be an assertion. > >> If these are not likely due to user input issues, then perhaps they should >> all be assert so that they are compiled out in release compilers? > > Since all these problems are reported by my static analyzer, I do not really > know whether these checked pointers will actually be null when the code is > executed. And I did not try to dynamically execute the program to check the > problems either. But chances are that if the creator callbacks are not > properly set or the called creator functions returns nullptr, the problem > will happen. In my opinion, these problems may only happen during > development. Therefore, I believe asserts can be sufficient to diagnose the > problems. > > If you think it would be better to use assertions instead of fatal errors, I > will make an update on all `llvm/lib/xxx` files (or maybe all files). But > before that, I'd prefer waiting for the replies from other reviewers on the > remaining parts of the patch and making an update for all the suggestions. Sure that sounds good. (Otherwise the llvm/lib parts LGTM) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91410/new/ https://reviews.llvm.org/D91410 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
