abstract method decls; explicit_method_decls provisional feature#386
abstract method decls; explicit_method_decls provisional feature#386lwaern-intel wants to merge 5 commits intointel:mainfrom
explicit_method_decls provisional feature#386Conversation
|
PR Verification: ✅ success |
py/dml/structure.py
Outdated
| assert abstract_decls | ||
| for decl in abstract_decls: | ||
| # We favor reporting EABSTEMPLATE (done by the caller) | ||
| # over EABSMETH | ||
| if not decl.shared: | ||
| report(EABSMETH(decl.site, name)) |
There was a problem hiding this comment.
It would maybe be cleaner if checking abstract methods+params was a single responsibility? The overlap between EABSMETH and EABSTEMPLATE feels somewhat unnatural, maybe it would suffice with EABSMETH and ENPARAM, and give both an optional template arg? or even a mandatory template arg, that may be a file?
In any case: no need to fix within this PR.
py/dml/traits.py
Outdated
| decl_ancestry_map = {} | ||
| for (r, anc_ranks) in minimal_ancestry.items(): | ||
| anc_decls = [anc_decl | ||
| for anc in anc_ranks | ||
| for anc_decl in flattened(rank_to_method[anc])] | ||
|
|
||
| decls = flattened(rank_to_method[r]) if r is not None else (None,) | ||
| for decl in decls: | ||
| decl_ancestry_map[decl] = anc_decls | ||
|
|
||
| method_map_ranks = {} | ||
|
|
||
| def calc_highest_impls(r): | ||
| existing = method_map_ranks.get(r) | ||
| if existing is not None: | ||
| return | ||
|
|
||
| anc_impl_ranks = Set() | ||
| some_abstract = False | ||
| for anc in minimal_ancestry[r]: | ||
| [anc_impl, anc_abstracts] = rank_to_method[anc] | ||
| if anc_impl is not None: | ||
| anc_impl_ranks.add(anc) | ||
| else: | ||
| some_abstract = True | ||
| calc_highest_impls(anc) | ||
| anc_impl_ranks.update(method_map_ranks[anc]) | ||
|
|
||
| if some_abstract: | ||
| anc_impl_ranks = get_highest_ranks(anc_impl_ranks) | ||
| method_map_ranks[r] = anc_impl_ranks | ||
|
|
||
| calc_highest_impls(None) | ||
| for (r, (impl, abstracts)) in rank_to_method.items(): | ||
| if (impl is not None and not impl.shared | ||
| and impl.site.provisional_enabled( | ||
| provisional.explicit_method_decls)): | ||
| existing = abstracts or decl_ancestry_map[impl] | ||
| if impl.explicit_decl: | ||
| if existing: | ||
| report(EOVERRIDEMETH(impl.site, existing[0].site, | ||
| impl.name, | ||
| 'default ' * impl.overridable)) | ||
| elif not existing: |
There was a problem hiding this comment.
If I understand correctly, you are mutating three dicts in multiple phases. I find it somewhat hard to follow. Can it be improved somehow? e.g., if mutation spaghetti is necessary, then encapsulate it further into a function. Alternatively, if it's not too expensive to create one of the tables at a time, then that could also be an option.
Also, on the surface this looks like a bit more code than before to execute per method, so we might want a quick check that it doesn't visibly degrade performance.
There was a problem hiding this comment.
Discussed verbally. I've now simplified the implementation with some comprimises (performance and maybe-possibly a DML 1.2 exclusive risk when used with DML 1.4 code with abstract decls)
lwaern-intel
force-pushed
the
lw/explicit_method_decls
branch
from
dc3cf35 to
9856a4f
Compare
lwaern-intel
force-pushed
the
lw/explicit_method_decls
branch
from
9856a4f to
bd207f5
Compare
|
Your last change actually seems to have improved the performance somewhat, from ~18 to ~16, on my crude and questionable benchmark. The number before this PR was ~11. |
|
looks good and clean overall, only found some minor remarks |
Namely the inability to handle 1.2 transformations, and perhaps performance
lwaern-intel
force-pushed
the
lw/explicit_method_decls
branch
from
bd207f5 to
144915d
Compare
3 participants
I'll split up the commit into two (one for
explicit_method_decls; one for abstract method decls) once this is ready to merge