Solution to conditionally dump overrides#127
Solution to conditionally dump overrides#127amir734jj wants to merge 11 commits intoboyland:masterfrom
Conversation
amir734jj
changed the title
amir734jj
changed the title
boyland
left a comment
boyland
left a comment
There was a problem hiding this comment.
But see if we can find counter-examples before changing things in this PR
|
|
||
| extern Declaration module_TYPE; | ||
| extern Declaration module_PHYLUM; | ||
|
|
There was a problem hiding this comment.
These two global static variables were originally defined for canonical stuff. So I thought let's reuse them.
aps2scala/dump-scala.cc
Outdated
| switch (Declaration_KEY(item)) { | ||
| case KEYsome_function_decl: | ||
| if (!strcmp( | ||
| symbol_name(def_name(some_function_decl_def(item))), |
There was a problem hiding this comment.
Can't you compare symbols here? That's teh point of symbols: faster than comparing strings. symbols are "eq" is equal (or there's a bug).
aps2scala/dump-scala.cc
Outdated
| dump_function_prototype(name,fty,oss); | ||
| Declaration mdecl = get_enclosing_some_class_decl(decl); | ||
| std::set<Declaration> visisted; | ||
| dump_function_prototype(name,fty, check_override_decl(some_class_decl_result_type(mdecl), decl, visisted), oss); |
There was a problem hiding this comment.
To me this looks like you might be adding overrides to functions in class declarations. That seems fishy to me. But it should cause many problem if it's a problem. Does basic.sala get created correctly?
Also: what if the function is at the top-level? I'd expect this code to crash. It doesn't crash?
| } | ||
|
|
||
| return check_override_decl(result, fdecl, visited); | ||
| } |
There was a problem hiding this comment.
Not quite. This should check the rtype, or you might miss something. Let me see if I can create a counter-example, before you change anything. If my guess is right (and it might not be), then the example will miss an override.
amir734jj
changed the title
boyland
left a comment
boyland
left a comment
There was a problem hiding this comment.
Looks pretty good, but:
- What if the same decl name is used with a different return type? If that's a Scala error, then is the APS bad too? I would think so? Or does APS allow overriding? I think not.
check_override_declseems better named something like: "type_has_service_function" or something that says it is checking whether this type exports a service function (not a value?) with the given name (and maybe with the given return type).- The name should reflect that it is asking a question about the APS, not about Scala per se and how APOS is implemented. (overriding)
aps2scala/dump-scala.cc
Outdated
| } | ||
| } | ||
|
|
||
| bool check_override_decl(Declaration decl, Declaration fdecl) { |
There was a problem hiding this comment.
Can you give a quick synopsis of what this function does?
There was a problem hiding this comment.
Perhaps refactor to just take a Symbol for the second parameter, and rename the function as "hasExport" or something, and then the synopsis is that the type provides a function service with the given name.
…s bad invalid scala code to be generated
2 participants
This PR generates working scala code for both
flat.apsandover.aps