Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.
/ druntime Public archive

Remove unused rt.typeinfo.ti_AC#1782

Merged
dlang-bot merged 1 commit intodlang:masterfrom
schveiguy:removeti_ac
Mar 2, 2017
Merged

Remove unused rt.typeinfo.ti_AC#1782
dlang-bot merged 1 commit intodlang:masterfrom
schveiguy:removeti_ac

Conversation

@schveiguy
Copy link
Member

@schveiguy schveiguy commented Mar 2, 2017

Can't find it anywhere in dmd sources, and it seems to have a non-standard implementation for comparison (lexicographical comparison is not used). Is it ever used? If so, this should show when it is.

DO NOT MERGE, this is only a test to see if it's even needed. If it's not needed, possibly we can merge this.

Now, I'm pretty sure it's never used. I think we can drop this file and TypeInfo instance.

@schveiguy schveiguy changed the title DO NOT MERGE see if ti_AC.d is even used Remove unused ti_AC (TypeInfo_AC) Mar 2, 2017
Copy link
Member

@andralex andralex left a comment

Choose a reason for hiding this comment

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

awes

@andralex
Copy link
Member

andralex commented Mar 2, 2017

Can you please do the same for Typeinfo_ag?

@schveiguy
Copy link
Member Author

Can you please do the same for Typeinfo_ag?

Done, actually just added it to this PR.

@CyberShadow CyberShadow changed the title Remove unused ti_AC (TypeInfo_AC) Remove unused rt.typeinfo.ti_AC and .ti_Ag modules Mar 2, 2017
@CyberShadow
Copy link
Member

Commit messages aren't very descriptive, grumble grumble. Doesn't matter here because they'll be squashed and the PR title will be used (I've updated that).

@CyberShadow
Copy link
Member

I think ti_Ag actually is used though:

/home/ubuntu/druntime/generated/linux/debug/64/libdruntime.a(exception.o): In function `_D4core9exception13FinalizeError8toStringMxFNfZAya':
/home/ubuntu/druntime/src/core/exception.d:139: undefined reference to `_D12TypeInfo_Aya6__initZ'

@CyberShadow
Copy link
Member

I think @WalterBright meant just the TypeInfo_Ag class, not the entire ti_Ag module.

@schveiguy
Copy link
Member Author

@CyberShadow In my defense, the original commit was just a test, I was actually expecting to see compiler failures (well, much like there are now. Apparently ti_Ag.d is used).

@WalterBright
Copy link
Member

I'm looking to see where that reference is generated.

@schveiguy
Copy link
Member Author

I'm pretty sure nothing in the ti_Ag file would be possible without the TypeInfo_Ag type.

@schveiguy
Copy link
Member Author

schveiguy commented Mar 2, 2017

It's TypeInfo_Aya (commented as string) that is used somewhere.

@CyberShadow
Copy link
Member

I'm pretty sure nothing in the ti_Ag file would be possible without the TypeInfo_Ag type.

Right :)

@WalterBright
Copy link
Member

Right, I'm just trying to find the special case in the compiler that generates it.

@WalterBright
Copy link
Member

WalterBright commented Mar 2, 2017

Ah, found it in typinf.d:

/* These decide if there's an instance for them already in std.typeinfo,
 * because then the compiler doesn't need to build one.
 */
extern (C++) static bool builtinTypeInfo(Type t)
{
    if (t.isTypeBasic() || t.ty == Tclass || t.ty == Tnull)
        return !t.mod;
    if (t.ty == Tarray)
    {
        Type next = t.nextOf();
        // strings are so common, make them builtin
        return !t.mod &&
               (next.isTypeBasic() !is null && !next.mod ||
                next.ty == Tchar && next.mod == MODimmutable ||
                next.ty == Tchar && next.mod == MODconst);
    }
    return false;
}

@CyberShadow
Copy link
Member

So... ti_Ag stays?

@schveiguy
Copy link
Member Author

It may be better to have ti_Ag stay, and then remove it later if/when the "special case" is removed.

@schveiguy
Copy link
Member Author

Other option is to manually rewrite TypeInfo_Aya into a single class instead of the whole inheritance tree that isn't used anywhere else. In any case, best left to another PR.

@WalterBright
Copy link
Member

yeah, I agree. Worry about it later.

@schveiguy schveiguy changed the title Remove unused rt.typeinfo.ti_AC and .ti_Ag modules Remove unused rt.typeinfo.ti_AC Mar 2, 2017
@schveiguy
Copy link
Member Author

@CyberShadow fixed the commit message ;)

@schveiguy
Copy link
Member Author

@WalterBright Do you want to make a bug report for the "special case", so it's not forgotten?

@schveiguy
Copy link
Member Author

I think there are more than just TypeInfo_Aya that are used in that file. If I look at the coverage report on this page: https://github.com/dlang/druntime/blob/master/src/rt/typeinfo/ti_Ag.d

I can see a lot of green, and many of the next members (which are all overridden) are green, which means something used them somewhere.

@dlang-bot dlang-bot merged commit 4025618 into dlang:master Mar 2, 2017
@wilzbach
Copy link
Contributor

wilzbach commented Mar 2, 2017

If I look at the coverage report

FYI: CodeCov also provides a nice UI to browse the coverage of files:

It can be reached via https://codecov.io/gh/dlang/druntime and then selecting a specific commit.
For example:

https://codecov.io/gh/dlang/druntime/tree/86b0795f65e887d38dac33b175a7095d9c3a7437/src

So there are a couple of other files that show up with 0% coverage - in some cases more rightfully than in others:

  • src/core/internal/abort.d
  • src/rt/obj.d
  • src/rt/typeinfo/ti_C.d
  • src/rt/typeinfo/ti_ireal.d
  • src/rt/typeinfo/ti_ilong.d
  • src/rt/typeinfo/ti_n.d
  • src/rt/typeinfo/ti_ptr.d
  • src/rt/tracegc.d (shouldn't there be a test for this?)
  • src/core/stdc (yep there are some functions in there as well)
  • src/core/sync/exception.d (SyncError)
  • src/rt/backtrace/elf.d (apparently it's possible to load an Elf files on Linux, but we never test it)

@schveiguy schveiguy deleted the removeti_ac branch March 3, 2017 13:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants