Adding some shape to toolchain semantic analysis #1092
Conversation
There was a problem hiding this comment.
(just a brief note since this has been silent too long, I've read this PR now, and am thinking through it a little bit... as soon as I have anything coherent, I'll say so, but I it is a useful pause point, and I'm understanding a bit more of the structure you're outlining... additionally, maybe good to just grab some live discussion time, will see if there is a good opportunity for that.)
|
I've tried to reorient this based on conversation:
There's still no file separation, but my leaning was to start here... I feel that splitting out a file shouldn't be too bad later. |
|
Note, part of my uncertainty here stems from pacing -- it's been several weeks and I've lost context that came from initial implementation. This was just the starting point, so I've lost a lot. If you want me to go away and build a more complete implementation before approving anything, I can. But I would need to reinvest significant time again. |
There was a problem hiding this comment.
Generally I think this is a fine increment -- definitely want ot unblock and let you flessh out more of the details here, thanks for answering my questions just to confirm my understanding of the code. Approving so you're not blocked on me, and some fairly tactical things inline.
| // will be indexed into. | ||
| enum class Kind { | ||
| Invalid, | ||
| Function, |
There was a problem hiding this comment.
Whether here or in a future step, I might call this a FunctionDeclaration or if you want a verb formulation DeclareFunction. Again, not at all blocking, just a thought for going forward.
There was a problem hiding this comment.
I'll figure out separately if this needs to be separated out -- I was still considering how to determine the body (tempting is having them be the same, with a separation only on the body, particularly if that's how they work syntactically)
toolchain/parser/parse_tree.h
Outdated
| // Returns true if the node is valid; in other words, it was not default | ||
| // initialized. | ||
| auto is_valid() -> bool { return index_ != -1; } | ||
|
|
There was a problem hiding this comment.
Is it worth separating the validity changes into a separate PR? Not a big deal, looks trivial either way, but would be good to add a unittest to parse_tree that covers this?
This comes from #1092 where I wrote bad code and I felt it should have been caught more clearly.
This comes from #1092 where I wrote bad code and I felt it should have been caught more clearly.
I've been trying to figure out a good architecture around the "command" semantics suggested. https://github.com/carbon-language/carbon-lang/pull/1092/files/f70e5bc359eebef1174bb4153d7c8690eb788fe4 shows one approach that had the analyzer more tightly part of the Semantics object. The current approach pulls it out more, trying to set it up something SemanticAnalyzer is processing the ParseTree, while Semantics would create the top-level objects.
e.g., probably something like Function::AddParameter and similar does object-specific manipulations. So in the example of a parameter, Semantics::AddVariable would definition the variable in the function's scope, and then Function::AddParameter would take the created reference and add it to the parameter list (although maybe these would be intertwined more). Something like a code block I think is processed within SemanticAnalyzer, spinning off calls to Semantics to construct the scope as it encounters node.
I've been mulling ways to extract out Function more. I could put it in as Carbon::SemanticFunction. I'm nervous about Carbon::Function, though, and I don't think you'd want Carbon::SomeNamespace::Function on principle. One option has been to keep it in the class as Semantics::Function, but still split the declaration/definition out to its own header. I think that kind of split would work -- although it may need to remain one build target.
I'm not sure if this matches what you were thinking -- I've been wondering and figured this would be a different pause point as the structure starts fleshing out.