CHECK that PreCheck and GetCheckUnit are not called twice #4608
Conversation
| @@ -420,6 +420,8 @@ class CompilationUnit { | |||
|
|
|||
| auto PreCheck() -> Parse::NodeLocConverter& { | |||
| CARBON_CHECK(parse_tree_, "Must call RunParse first"); | |||
| CARBON_CHECK(!node_converter_.has_value(), "Called PreCheck twice"); | |||
There was a problem hiding this comment.
In other calls (RunLex, RunParse, PostCheck, etc) we only verify that we're moving forward. They would share this problem. I'm fine with adding more checks to help document the progressive call structure, but I think it's implying the wrong thing to only check in some calls: can you please add them in the other spots too?
There was a problem hiding this comment.
Right, these functions do not emplace any optionals, so it's not clear how to CHECK that without adding a bool for the purpose of checking, or maybe an enum tracking the progress through them all if it's linear. WDYT?
There was a problem hiding this comment.
Note, we typically don't write .has_value() for optionals. parse_tree_ is an optional [for example]. You could use the same CHECKs we're using to ensure things are called in order, to make sure they're not called again.
There was a problem hiding this comment.
Oh, thanks, will do. The lack of has_value() and use of operator= had me miss that. There's a couple functions that are still missed, but this covers most of them (and all the ones that set optionals).
If they were called twice for a snigle CompilationUnit, they would destroy objects that they created and returned a pointer to, leaving a dangling pointer somewhere else.
danakj
force-pushed
the
precheck-checkunit-twice
branch
from
eab4d30 to
9f16edd
Compare
If they were called twice for a CompilationUnit, they would destroy objects that they created and returned a pointer to, leaving a dangling pointer somewhere else.