Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix Issue 16495 - __traits(fullyQualifedName) instead of std.traits.f… #14711

Merged
merged 1 commit into from
Jan 22, 2023

Conversation

WalterBright
Copy link
Member

…ullyQualifiedName

As the bug report suggests, this is better done in the compiler than with complex metaprogramming which is a bit of a speed problem. The library template also duplicates code which must be in the compiler anyway, why do it twice.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close Bugzilla Severity Description
16495 enhancement __traits(fullyQualifedName) instead of std.traits.fullyQualifiedName

Testing this PR locally

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

dub run digger -- build "master + dmd#14711"

@WalterBright
Copy link
Member Author

Will add more tests later.

@ljmf00
Copy link
Member

ljmf00 commented Dec 18, 2022

I'm in favor of this change. I would like to point out there's also another problematic trait that is way simpler to implement in the compiler and increase a lot compile-times. moduleName and a derivative implementation of it, moduleSymbol, is way harder to implement as a library, and in some cases is actually impossible, because the compiler yields a forward reference error due to __traits(parent) requiring to resolve the types of that symbol. I would suggest to add a __traits(moduleSymbol, symbol) too.

@ljmf00 ljmf00 added Needs Changelog A changelog entry needs to be added to /changelog Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org Needs Tests labels Dec 18, 2022
@WalterBright
Copy link
Member Author

@lmjf00 Please file enhancement requests in bugzilla.

@RazvanN7
Copy link
Contributor

Will add more tests later.

@WalterBright any progres on this?

@WalterBright
Copy link
Member Author

I added the tests from std.traits. But the results aren't identical, as there are multiple ways to print the same fully qualified name.

Copy link
Contributor

@RazvanN7 RazvanN7 left a comment

Choose a reason for hiding this comment

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

Also, please replaces the pragma(msg)s with static asserts in test16495.d.


Scope* sc2 = sc.push();
sc2.flags = sc.flags | SCOPE.noaccesscheck | SCOPE.ignoresymbolvisibility;
bool ok = TemplateInstance.semanticTiargs(e.loc, sc2, e.args, 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is redundant. `TemplateInstance.semanticTiargs is already called at the beginning of this function.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not quite the same. The sc2.flags are changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but since semantic has already been performed all of the expressions have their type set and when they hit the expression semantic visit methods (which typically start with if (exp.type) return) no analysis is done. So, I suspect that this code only adds a couple of function calls that simply return.

compiler/src/dmd/traits.d Outdated Show resolved Hide resolved
@RazvanN7
Copy link
Contributor

Could we start deprecating std.traits.fullyQualifiedName?

@WalterBright
Copy link
Member Author

Could we start deprecating std.traits.fullyQualifiedName?

Better to just re-implement it as a shell calling __traits.

@WalterBright WalterBright force-pushed the fix16495 branch 2 times, most recently from 5dadfd6 to 2465dca Compare January 18, 2023 23:49
@WalterBright
Copy link
Member Author

Documentation: dlang/dlang.org#3495

@adamdruppe
Copy link
Contributor

As I've said many many times, any code that calls this is questionable. There's no real benefit to improving a function that should never actually be used.

I'm not against it per se, but.... if people think they need this, they're almost certainly wrong.

@RazvanN7 RazvanN7 removed Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org Needs Tests labels Jan 19, 2023
@RazvanN7
Copy link
Contributor

@WalterBright Just add a small changelog entry so that users of the next release can find out about this feature and we are good to go. Thanks!

@dkorpel
Copy link
Contributor

dkorpel commented Jan 20, 2023

There's still test coverage missing for failure cases

@adamdruppe
Copy link
Contributor

Why are we adding more useless cruft to the language when there clear cases of harm done by it that is avoided by easy to use alternative techniques?

@WalterBright
Copy link
Member Author

failure cases

Those are kinda hard to figure out how to trigger.

@WalterBright WalterBright merged commit 2e196ae into dlang:master Jan 22, 2023
@WalterBright WalterBright deleted the fix16495 branch January 22, 2023 05:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Needs Changelog A changelog entry needs to be added to /changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants