Skip to content
This repository has been 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
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

radcapricorn
Copy link
Contributor

@radcapricorn radcapricorn commented Nov 24, 2018

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
Contributor

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
Copy link
Member

n8sh commented Nov 24, 2018

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
Contributor Author

That's actually a requirement for fixing it :)

Copy link
Contributor

@thewilsonator thewilsonator left a comment

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

@@ -1028,18 +1028,31 @@ struct OffsetTypeInfo
TypeInfo ti; /// TypeInfo for this member
}

enum ToStringContext
Copy link
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
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
Contributor

Choose a reason for hiding this comment

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

@dlang-bot dlang-bot added Needs Rebase needs a `git rebase` performed stalled labels May 27, 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 stalled
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants