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

Add __traits(getLocation) #10013

Merged
merged 5 commits into from
Jun 26, 2019
Merged

Add __traits(getLocation) #10013

merged 5 commits into from
Jun 26, 2019

Conversation

thewilsonator
Copy link
Contributor

@thewilsonator thewilsonator commented Jun 10, 2019

@thewilsonator thewilsonator added Needs Changelog A changelog entry needs to be added to /changelog WIP Work In Progress - not ready for review or pulling Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org labels Jun 10, 2019
@TurkeyMan
Copy link
Contributor

Oh wow, perfect. Thanks!!

src/dmd/traits.d Outdated Show resolved Hide resolved
@thewilsonator
Copy link
Contributor Author

Oh wow, perfect. Thanks!!

No probs, you're writing the test suite for it though!

Specifically all the cases that look like symbols but actually end up as something else e.g. DsymbolExp is an expression (i've no idea how to get one of them though), structs and classes are types (their declarations are dsymbols, but they aren't). There's probably a whole lot more edge cases like that.

@thewilsonator
Copy link
Contributor Author

And things like overload sets.

src/dmd/traits.d Outdated
}

auto exps = new Expressions(3);
exps.data[0] = new StringExp(e.loc, cast(char*)s.loc.filename);
Copy link
Member

Choose a reason for hiding this comment

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

use the void*,size_t overload.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should the string be mem.xdup'ed too? I wasn't sure if e.g. ctfe screws with the contents.

src/dmd/traits.d Outdated Show resolved Hide resolved
@thewilsonator thewilsonator force-pushed the traits-getLoc branch 4 times, most recently from 09305f9 to ef2c455 Compare June 10, 2019 06:46
Copy link
Contributor

@jacob-carlborg jacob-carlborg left a comment

Choose a reason for hiding this comment

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

  • Would be nice to have a test that tests all elements returned and not just first one
  • Needs a changelog as well.

src/dmd/traits.d Outdated Show resolved Hide resolved
src/dmd/traits.d Outdated Show resolved Hide resolved
test/compilable/traits.d Show resolved Hide resolved
@thewilsonator thewilsonator removed the Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org label Jun 13, 2019
@thewilsonator thewilsonator marked this pull request as ready for review June 14, 2019 03:17
@thewilsonator thewilsonator dismissed jacob-carlborg’s stale review June 14, 2019 03:17

changelog added, test for line and column added

@thewilsonator thewilsonator removed Needs Changelog A changelog entry needs to be added to /changelog WIP Work In Progress - not ready for review or pulling labels Jun 14, 2019
changelog/getloc.dd Outdated Show resolved Hide resolved
src/dmd/traits.d Outdated Show resolved Hide resolved
@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @thewilsonator! 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 + dmd#10013"

@thewilsonator thewilsonator force-pushed the traits-getLoc branch 2 times, most recently from 560e48b to 258477f Compare June 16, 2019 07:13
@@ -4,6 +4,7 @@
// effort to keep the number of files in the `compilable` folder to a minimum.

// https://issues.dlang.org/show_bug.cgi?id=19152
module traits;
Copy link
Member

Choose a reason for hiding this comment

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

What happens if the module name is not explicitly specified? I would expect the module name to be inferred from the file name and things to just work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so, this was just out of habit to add a module declaration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked, you get verbatim what was passed on the command line .././../foo/./../file.d and all.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for checking, indeed we should use an explicit module name here in that case.

@thewilsonator thewilsonator changed the title Add __traits(getLoc) Add __traits(getLocation) Jun 16, 2019
@thewilsonator thewilsonator added the 72h no objection -> merge The PR will be merged if there are no objections raised. label Jun 16, 2019
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Please add handling of overloads. A mention in the changelog is not enough and will be forgotten soon.

Test case accompanying the suggested changes:

module trait_loc_ov_err;
/*
TEST_OUTPUT:
---
fail_compilation/trait_loc_ov_err.d(25): Error: cannot get location of an overload set, use `__traits(getOverloads, ..., "ov1")[N]` to get the Nth overload location
fail_compilation/trait_loc_ov_err.d(26): Error: cannot get location of an overload set, use `__traits(getOverloads, ..., "ov2")[N]` to get the Nth overload location
fail_compilation/trait_loc_ov_err.d(27): Error: cannot get location of an overload set, use `__traits(getOverloads, ..., "OvT(T)")[N]` to get the Nth overload location
---
*/

void ov1(){}
void ov1(int){}

void ov21(){}
void ov22(int){}
alias ov2 = ov21;
alias ov2 = ov22;

template OvT(T){}
template OvT(T, U){}

enum e1 = __traits(getLocation, ov1);
enum e2 = __traits(getLocation, ov2);
enum e3 = __traits(getLocation, OvT);

src/dmd/traits.d Show resolved Hide resolved
@thewilsonator
Copy link
Contributor Author

Done.

@thewilsonator
Copy link
Contributor Author

Hmm, so

template OvT(T, U){}
template OvT(T){}
enum e9  = __traits(getLocation, __traits(getOverloads, trait_loc_ov_err, "OvT", true)[1]);
enum e10 = __traits(getLocation, __traits(getOverloads, trait_loc_ov_err, "OvT", true)[0]);

will always give an error because it seems that using templates with __traits(getOverloads) doesn't remove the symbol from the overload set, that is td.overnext is always non-null for the first template.

I'm tempted to just define it to always return the first overload of the set, or even just add an integer option to select which overload as part of the trait.

Any thoughts, ideas, opinions?

@ghost
Copy link

ghost commented Jun 17, 2019

Any thoughts, ideas, opinions?

Yes, my suggestion would be to comment the handling of td and leave a note : FIXME:td.overnext is always set, even when using an index on it. I've tried to set overnext to null in expressionsem for IndexExp but that does not work so far (or create bounds CT error), although it would be the place to do that. The problem is that it's possible that the overload set would be something not to touch, i.e I'm not sure it's rebuild each time a symbol lookup is made...

@ghost
Copy link

ghost commented Jun 17, 2019

@thewilsonator : abandoning the detection of template overloads is the way to go.

Its impossible to tell it the user has used
`__traits(getOverloads,…,true)`
@thewilsonator
Copy link
Contributor Author

Finally got around to it.

@thewilsonator thewilsonator added auto-merge-squash and removed 72h no objection -> merge The PR will be merged if there are no objections raised. labels Jun 26, 2019
@adamdruppe
Copy link
Contributor

is this in the new release? looks like no...

@Geod24
Copy link
Member

Geod24 commented Jul 4, 2019

No it did not make the cut.

@baryluk
Copy link

baryluk commented May 10, 2020

Hi, that is amazing and super useful. Could we also get a simple wrapper in std.traits for this? I was looking at std.traits, and could find info, and I almost finished writing a bug report to report filename / line for the symbol, but there is mechanism for it in compiler, just not in std.traits, which makes it harder to find. Maybe the wrapper could format it into "filename:line:column" string automatically, and make a note about using __traits directly to get structured info?

@thewilsonator thewilsonator deleted the traits-getLoc branch July 1, 2021 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.