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

Tentative fix for Issue 7270 - needless qualifiers in TypeInfo.toString#2374

Open
radcapricorn wants to merge 1 commit into
dlang:masterfrom
radcapricorn:typeInfoToStringQualifiers
Open

Tentative fix for Issue 7270 - needless qualifiers in TypeInfo.toString#2374
radcapricorn wants to merge 1 commit into
dlang:masterfrom
radcapricorn:typeInfoToStringQualifiers

Conversation

@radcapricorn

@radcapricorn radcapricorn commented Nov 24, 2018

Copy link
Copy Markdown
Contributor

I don't like this approach too too much, but something needs to be done, so it's a starting point. That issue is louder than it appears. If typeid(X) == typeid(Y), then it better be that typeid(X).toString() == typeid(Y).toString().
I'm open to suggestions on other approaches.
Need more tests. And feedback!

@dlang-bot

Copy link
Copy Markdown

Thanks for your pull request and interest in making D better, @radcapricorn! 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

Auto-close Bugzilla Severity Description
7270 enhancement Eliminate needless qualifiers in TypeInfo.toString

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#2374"

@n8sh

n8sh commented Nov 24, 2018

Copy link
Copy Markdown
Member

A possible problem with fixing 7270 is that base TypeInfo.opEquals uses string equality.

druntime/src/object.d

Lines 1060 to 1070 in e798185

override bool opEquals(Object o)
{
/* TypeInfo instances are singletons, but duplicates can exist
* across DLL's. Therefore, comparing for a name match is
* sufficient.
*/
if (this is o)
return true;
auto ti = cast(const TypeInfo)o;
return ti && this.toString() == ti.toString();
}

@radcapricorn

Copy link
Copy Markdown
Contributor Author

That's actually a requirement for fixing it :)

@thewilsonator thewilsonator left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This lacks tests, but otherwise LGTM

Comment thread src/object.d
TypeInfo ti; /// TypeInfo for this member
}

enum ToStringContext

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

DDOC comments

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yup, will do.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 stalled

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants