Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

Refactor TypeInfo_Array #2506

Closed
wants to merge 2 commits into from
Closed

Conversation

edi33416
Copy link
Contributor

@edi33416 edi33416 commented Mar 5, 2019

This is a first step towards refactoring TypeInfo_Array. Ideally, imho, we would replace all the inheritance chain with templates.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @edi33416! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + druntime#2506"

src/rt/typeinfo/ti_Afloat.d Outdated Show resolved Hide resolved
@jacob-carlborg
Copy link
Contributor

If you want this to be replaced with templates, is there any reason to do this refactoring? Will more of the code now be reusable if/when this code is replaced with templates?

@JinShil
Copy link
Contributor

JinShil commented Mar 6, 2019

Since I had, at one time, an interest in reducing druntime's dependency on TypeInfo, I was asked to comment on this PR, so here are my thoughts...

I'm not sure what's motivating this PR. It appears to be simply reducing some boilerplate and increasing code re-use. That's fine, but that does not address the fundamental issue with TypeInfo and why we want to use more templates in druntime.

The fundamental problem with druntime's use of TypeInfo has to do with the runtime hooks which are basically just compiler lowerings. For example, consider the following code:

void main()
{
    auto bArray = new byte[5];
    bArray.length = 8;  // gets lowered to `_d_arraysetlengthT`
}

bArray.length = 8; gets lowered by the compiler to the runtime hook _d_arraysetlengthT. If you look at the signature for _d_arraysetlengthT, you'll see that it takes a runtime TypeInfo object. That is unfortunate because everything the compiler needs for the implementation is known at compile-time. Therefore, _d_arraysetlengthT should be converted to a template, and the compiler should be changed to lower bArray.length = 8; to that template implementation instead. There are a number of benefits that the template implementation provides, some of which should be self-evident IMO, so I won't elaborate at this time.

Although it was before my time, my understanding for why things are currently implemented with runtime TypeInfo when everything is known at compile-time is because the runtime hooks were written before D had templates. Now that D has templates, it seems, at least to me, that templates is the obvious choice.

There is another problem, however, when converting these runtime hooks to templates. In the compiler, the lowerings are performed after the semantic phase in e2ir.d. This means that currently, for example, setting the length of a dynamic array can be done in a @safe, pure, nothrow context, but _d_arraysetlengthT is neither @safe, pure, nor nothrow. The compiler is lying to us. Therefore, if you convert these runtime hooks to templates, the lowering will be done in the semantic phase, and suddenly, you'll be forced to be honest. After a conversation with @WalterBright, I was told to make use of pureMalloc and friends to work around the pure constraint. trusted can be used to work around the @safe constraint, and that instead of throwing exceptions in nothrow code, to use assertions.

So, I don't know what's motivating this PR, but it doesn't seem to address the fundamental problem with TypeInfo in the runtime. It may still be an improvement to the code, but I'll leave that for others to decide.

/**
* TypeInfo support code.
*
* Copyright: Copyright Digital Mars 2004 - 2009.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update year to 2019

src/rt/util/typeinfo.d Outdated Show resolved Hide resolved
private template Array(T)
if (isIntegralTypeInfoT!T)
{
bool equals(T[] s1, T[] s2)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally there'd be some @trusted here and stuff.

@codecov
Copy link

codecov bot commented May 1, 2019

Codecov Report

Merging #2506 into master will increase coverage by 0.67%.
The diff coverage is 82.6%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2506      +/-   ##
==========================================
+ Coverage    73.7%   74.38%   +0.67%     
==========================================
  Files         146      137       -9     
  Lines       16607    16426     -181     
==========================================
- Hits        12241    12219      -22     
+ Misses       4366     4207     -159
Impacted Files Coverage Δ
src/rt/typeinfo/ti_A.d 100% <100%> (ø)
src/rt/util/typeinfo.d 73.33% <70.37%> (+0.47%) ⬆️
src/rt/lifetime.d 84.53% <0%> (-0.11%) ⬇️
src/core/internal/hash.d 80.72% <0%> (+0.23%) ⬆️
src/rt/util/container/treap.d 81.34% <0%> (+0.74%) ⬆️
src/rt/monitor_.d 54.45% <0%> (+1.98%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a8a602...f837f14. Read the comment docs.

@dlang-bot dlang-bot added Needs Rebase needs a `git rebase` performed Needs Work stalled labels May 27, 2021
@RazvanN7 RazvanN7 closed this Dec 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Needs Rebase needs a `git rebase` performed Needs Work stalled
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants