Fix formatting of forward declared generics #5530
Conversation
| static auto IsDefinitionStart(Parse::NodeKind node_kind) -> bool { | ||
| switch (node_kind) { | ||
| case Parse::NodeKind::BuiltinFunctionDefinitionStart: | ||
| case Parse::NodeKind::ChoiceDefinitionStart: | ||
| case Parse::NodeKind::ClassDefinitionStart: | ||
| case Parse::NodeKind::FunctionDefinitionStart: | ||
| case Parse::NodeKind::ImplDefinitionStart: | ||
| case Parse::NodeKind::InterfaceDefinitionStart: | ||
| case Parse::NodeKind::NamedConstraintDefinitionStart: | ||
| return true; | ||
| default: | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Is it possible to move this into parse somewhere in a way that makes it clear that it needs to be defined as true/false when adding new kinds of Nodes, so that Check can then just query it off the NodeKind?
There was a problem hiding this comment.
I don't think nodes have support like that, and this seems pretty minor to add new support, but maybe I'm missing something; can you give a little more detail about what you're suggesting?
There was a problem hiding this comment.
Well, I was thinking something like a bool is_definition_start() on the Parse::NodeKind struct here:
carbon-lang/toolchain/parse/node_kind.h
Line 34 in 3946cac
So it could be added to NodeKind::DefinitionArgs with a default value of false but overridden to true in typed_nodes.h.
There was a problem hiding this comment.
My perspective is that wouldn't need to be defined when adding new kinds, it just could be when people remember. We could do this by adding a NodeCategory, but I feel odd doing that when there's only one use, and any definition starts seem like they'll remain rare (and with handling specific to the formatter).
We also already have IsStartOfDeferredDefinitionScope and IsEndOfDeferredDefinitionScope in node_id_traversal.cpp, which feel similar; this keeps the check and logic close to the code using it.
How strongly do you feel here?
There was a problem hiding this comment.
If you don't think it's a win, then that's fine go with what you think is best. I thought that by having it in Parse, when adding something new I would at least have a hope of seeing all the things I need to set and think about which it is - and maybe copy/paste from a similar one too. Whereas over here, it's unlikely I will remember to set this correctly if I added a new node that should be true.
There was a problem hiding this comment.
I think this is probably better. FWIW, my thought is it might be easier to notice here when adding printing of a new entity. If the entity doesn't need formatting, then it also doesn't need handling here.
Noticed this while working on class tests (crash bug). Forward declared generics have a decl_id of the forward declaration, not the definition. I'm giving up trying to have the caller know if it's a start node, and instead just choosing based on the node kind.