Migrate remaining exits to FATAL_*_ERROR calls #704
Conversation
executable_semantics/common/error.h
Outdated
|
|
||
| #define FATAL_COMPILATION_ERROR(line) \ | ||
| FATAL_COMPILATION_ERROR_NO_LINE() << line << ": " | ||
|
|
||
| #define FATAL_INTERNAL_ERROR_NO_LINE() \ |
There was a problem hiding this comment.
What's the difference between this and CHECK?
There was a problem hiding this comment.
It's really just that I was hesitant to do CHECK(false), and that has an issue anyways that it doesn't guarantee LLVM_ATTRIBUTE_NORETURN as this does.
Would you prefer that I provide a version of CHECK() that's equivalent? That may have an advantage anyways because CHECK prints a stack, which may make sense with an internal error. I could structure it the same as this code, but use the CHECK output format. Do you have suggestions on a name?
There was a problem hiding this comment.
Perhaps a vanilla FATAL() alongside CHECK()? I've refactored towards this, along with combining stream handlers.
There was a problem hiding this comment.
llvm has llvm_unreachable, but I was a little wary of that (partly because it uses strings). UNREACHABLE also didn't quite feel right with me. But let me know if you'd prefer that naming structure, fundamentally it's a similar concept.
There was a problem hiding this comment.
I see. Yeah, I think FATAL makes sense, but I'm inclined to drop FATAL_INTERNAL_ERROR. That's partly because a line number is less likely to be diagnostically useful for internal errors, but mostly it's for social engineering reasons: I'd like to encourage expressing this sort of logic as an invariant assertion (with CHECK) rather than explicit control flow, because it's more concise and makes the program logic clearer (see the comment below). I think keeping the macro right next to CHECK, rather than here, will help with that.
There was a problem hiding this comment.
Removed. Per your comment, I'm assuming you're fine dropping line numbers in these cases, so I've included that.
executable_semantics/common/error.h
Outdated
|
|
||
| #define FATAL_COMPILATION_ERROR(line) \ | ||
| FATAL_COMPILATION_ERROR_NO_LINE() << line << ": " | ||
|
|
||
| #define FATAL_INTERNAL_ERROR_NO_LINE() \ |
There was a problem hiding this comment.
I see. Yeah, I think FATAL makes sense, but I'm inclined to drop FATAL_INTERNAL_ERROR. That's partly because a line number is less likely to be diagnostically useful for internal errors, but mostly it's for social engineering reasons: I'd like to encourage expressing this sort of logic as an invariant assertion (with CHECK) rather than explicit control flow, because it's more concise and makes the program logic clearer (see the comment below). I think keeping the macro right next to CHECK, rather than here, will help with that.
| << "internal error, expected a tuple value in pattern, not " << *v | ||
| << "\n"; | ||
| exit(-1); | ||
| FATAL() << "expected a tuple value in pattern, not " << *v; |
There was a problem hiding this comment.
This is an example of why I want to encourage CHECK for internal errors: if we had CHECK(v->Tag() == Value::Kind::TupleValue); up on line 275, we wouldn't need the switch statement, which would simplify the code while making the intent clearer.
I'm not asking you to make this change; just noting it as an example of the motivation for my other comment.
There was a problem hiding this comment.
Noted, but I think often we do just want something more like FATAL(), e.g. the previous edit on line 102.
| .add_separator() | ||
|
|
||
| // This is similar to CHECK, but is unconditional. |
There was a problem hiding this comment.
It might be worth explicitly documenting why you'd want to use this instead of CHECK(false).
| .add_separator() | ||
|
|
||
| // This is similar to CHECK, but is unconditional. |
| << "internal error, expected a tuple value in pattern, not " << *v | ||
| << "\n"; | ||
| exit(-1); | ||
| FATAL() << "expected a tuple value in pattern, not " << *v; |
There was a problem hiding this comment.
Noted, but I think often we do just want something more like FATAL(), e.g. the previous edit on line 102.
executable_semantics/common/error.h
Outdated
|
|
||
| #define FATAL_COMPILATION_ERROR(line) \ | ||
| FATAL_COMPILATION_ERROR_NO_LINE() << line << ": " | ||
|
|
||
| #define FATAL_INTERNAL_ERROR_NO_LINE() \ |
There was a problem hiding this comment.
Removed. Per your comment, I'm assuming you're fine dropping line numbers in these cases, so I've included that.
Oversight from #704, I'd had this change in the client but missed it because I ran git add in the wrong spot.
Adds FATAL for things that are most likely programming errors in executable_semantics.
Oversight from #704, I'd had this change in the client but missed it because I ran git add in the wrong spot.
Adds FATAL_INTERNAL_ERROR for things that are most likely programming errors in executable_semantics.