[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

probinson added a comment. dblaikie: > In the same way that an assert says "This condition should never be false" - > I use "should" in the same sense in both unreachable and assert, and I > believe that's the prevailing opinion of LLVM developers/the LLVM style guide. aaronballman: > I belie

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

shafik added a comment. In D135551#3853365 , @rnk wrote: > I think the status quo has real problems. We pretend that we can do both of > these: > > - Assert liberally, with the understanding that assertion failures lead to UB > (failed bad cast check, b

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

arsenm added inline comments. Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:598 } else { -assert(false && "Unhandled type in array initializer initlist"); +llvm_unreachable("Unhandled type in array initializer initlist"); } aa

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

rnk added a comment. I think the disagreement here highlights the need to have a serious discussion about the future of error handling across the LLVM project. As you say, it sounds like you're not going to reach agreement on this code review, so maybe the best short term next step is to land t

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

aaron.ballman added a comment. In D135551#3850511 , @dblaikie wrote: > In D135551#3850391 , @aaron.ballman > wrote: > >> In D135551#3850308 , @dblaikie >> wrote: >> >>>

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

dblaikie added a subscriber: rnk. dblaikie added a comment. In D135551#3850391 , @aaron.ballman wrote: > In D135551#3850308 , @dblaikie > wrote: > >> In D135551#3850266

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

aaron.ballman added a comment. In D135551#3850308 , @dblaikie wrote: > In D135551#3850266 , @inclyc wrote: > >> This makes sense! However I think `assert(0)` should not be used in this >> case, we could expose an

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

dblaikie added a comment. In D135551#3850266 , @inclyc wrote: > This makes sense! However I think `assert(0)` should not be used in this > case, we could expose another `llvm_unreachable`-like api and probably > `llvm_report_error` shall be fine. Are th

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

inclyc added a comment. This makes sense! However I think `assert(0)` should not be used in this case, we could expose another `llvm_unreachable`-like api and probably `llvm_report_error` shall be fine. Are there some changed assertions actually "Aspirationally unreachable" in this patch? Rep

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

aaron.ballman added a comment. In D135551#3850132 , @inclyc wrote: > In D135551#3849983 , @aaron.ballman > wrote: > >> In D135551#3849944 , @inclyc wrote: >> - Prefe

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

inclyc added a comment. In D135551#3849983 , @aaron.ballman wrote: > In D135551#3849944 , @inclyc wrote: > >>> - Prefer llvm_report_error() in any circumstance under which a code path is >>> functionally possibl

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

aaron.ballman added a comment. In D135551#3849944 , @inclyc wrote: >> - Prefer llvm_report_error() in any circumstance under which a code path is >> functionally possible to reach, but only in erroneous executions that >> signify a mistake on the part o

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

inclyc added a comment. > - Prefer llvm_report_error() in any circumstance under which a code path is > functionally possible to reach, but only in erroneous executions that signify > a mistake on the part of the LLVM developer elsewhere in the program. > - Prefer llvm_unreachable() in any circu

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

aaron.ballman added a comment. In D135551#3849816 , @dblaikie wrote: >>> Generally LLVM's pretty hard to fathom in a non-asserts build anyway, >>> right? (that's the first thing any of us do is reproduce with an assertions >>> build that may fail miles

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

dblaikie added a comment. >> Generally LLVM's pretty hard to fathom in a non-asserts build anyway, right? >> (that's the first thing any of us do is reproduce with an assertions build >> that may fail miles away from where a crash occurred because an invariant >> was violated much earlier) - th

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

aaron.ballman added a comment. In D135551#3847962 , @dblaikie wrote: > In D135551#3847607 , @aaron.ballman > wrote: > >> In D135551#3847444 , @dblaikie >> wrote: >> >>>

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

dblaikie added a comment. Because I was looking, some other `assert(0)` -> `llvm_unreachable` cleanups (though, yes, even the earliest cleanups include some assert(0) -> report_fatal_error, but for externally/user-reachable failures, like invalid bitcode, I think). Some of these are more blanke

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

dblaikie added a comment. In D135551#3847607 , @aaron.ballman wrote: > In D135551#3847444 , @dblaikie > wrote: > >> I thought this was settled quite a while ago and enshrined in the style >> guide: https://llvm

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

dexonsmith added a comment. I would think we could convert every assert(0) to either `llvm::report_fatal_error` (guaranteed trap) or `llvm_unreachable()` (trap or optimize, depending on CMAKE configuration). The C API usage checks seem like good candidates for the former. Also, not sure if eve

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

aaron.ballman added a comment. In D135551#3847444 , @dblaikie wrote: > I thought this was settled quite a while ago and enshrined in the style > guide: https://llvm.org/docs/CodingStandards.html#assert-liberally > > `assert(0)` should not be used if some

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

dblaikie added a comment. Can't seem to find an llvm-dev/commits discussion for r166821, but I remember discussing it several times before, perhaps this one happened on IRC and so we may not have any record of it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://revie

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

dblaikie added a comment. I thought this was settled quite a while ago and enshrined in the style guide: https://llvm.org/docs/CodingStandards.html#assert-liberally `assert(0)` should not be used if something is reachable. We shouldn't have a "this violates an invariant, but if you don't have a

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

inclyc added a comment. > I've left some comments in the review about examples of my concerns (it's not > an exhaustive review). Thanks @aaron.ballman ! I didn't quite understand the original meaning of this code here (e.g. libclang), and I have now removed the relevant changes. I think this p

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

inclyc updated this revision to Diff 466539. inclyc added a comment. Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135551/new/ https://reviews.llvm.org/D135551 Files: clang/include/clang/AST/Redeclarable.h clang/include/clang/

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

aaron.ballman added a comment. In D135551#3846603 , @inclyc wrote: > In D135551#3846592 , @aaron.ballman > wrote: > >> I don't know if that discussion reached a conclusion to move forward with >> this change --

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

inclyc added a comment. In D135551#3846592 , @aaron.ballman wrote: > I don't know if that discussion reached a conclusion to move forward with > this change -- my reading of the conversation was that efforts would be > better spend on fuzzing instead o

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

aaron.ballman added a comment. I don't know if that discussion reached a conclusion to move forward with this change -- my reading of the conversation was that efforts would be better spend on fuzzing instead of changing policy about using unreachable vs assert(0). In general, I'm a bit hesitan

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

inclyc updated this revision to Diff 466471. inclyc added a comment. Herald added a project: clang. Herald added a subscriber: cfe-commits. Fix CI issue Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135551/new/ https://reviews.llvm.org/D135551 Fil