Implement 'extern library' support for functions. #4220
Conversation
|
Depends on #4219, split off LibraryNameId to try to make this smaller. |
toolchain/check/handle_modifier.cpp
Outdated
| @@ -34,6 +34,8 @@ static auto DiagnoseNotAllowedWith(Context& context, Parse::NodeId first_node, | |||
| .Emit(); | |||
| } | |||
|
|
|||
| // Handles the main modifier keyword. If valid, adds it to the modifier set and | |||
There was a problem hiding this comment.
What's the main modifier keyword?
There was a problem hiding this comment.
I've tried to give a couple more examples of what this handles for modifiers with multiple keywords.
toolchain/check/handle_modifier.cpp
Outdated
| // Handles the main modifier keyword (either alone, such as `private`, or the | ||
| // first in a complex modifier, such as `extern library ...`). If valid, adds it | ||
| // to the modifier set and returns true. Otherwise, diagnoses and returns false. |
There was a problem hiding this comment.
This sounds to me like it's saying that in private abstract fn Foo(), this function handles the private but not the abstract, which doesn't seem right. After talking to josh11b I think I understand what this is trying to say; assuming this is correct, I would find it much clearer:
| // Handles the main modifier keyword (either alone, such as `private`, or the | |
| // first in a complex modifier, such as `extern library ...`). If valid, adds it | |
| // to the modifier set and returns true. Otherwise, diagnoses and returns false. | |
| // Handles a modifier keyword, which may be a complete modifier (such as `private`), or the | |
| // first token of a complex modifier (such as `extern library ...`). If valid, adds it | |
| // to the modifier set and returns true. Otherwise, diagnoses and returns false. |
There was a problem hiding this comment.
In the extern library modifier, library is also a keyword. I would think a keyword that helps form a modifier is a "modifier keyword". However, this is not called for the "library" modifier keyword. So I think saying this handles "a modifier keyword" could be misleading.
I've tried a different phrasing, does that work for you?
Support for types (particularly classes) is left as a TODO.
There's also an issue I'm observing with a "define in impl" test, but this is probably an issue with resolving the prior declaration which is imported indirectly. The PR was already feeling big, so I'm choosing to cut here.
Note, this does not implement the rule "The owning library's API file must import the
externdeclaration, and must also contain a declaration."