Issue 16485 - Add trait for testing if a member is static. #5112

skl131313 · 2017-02-10T07:49:35Z

Opening this new pull request as @jmdavis seems busy and hasn't replied to pings, credit to him (#4834)

RazvanN7 · 2017-02-10T08:28:00Z

LGTM

ghost

No without the required change to the protection when certain __trait() verbs are used. Which is the case here.

ghost · 2017-02-10T22:16:49Z

std/traits.d

+template isStaticMember(T, string member)
+    if (__traits(hasMember, T, member))
+{
+    import std.meta : Alias;


nope, it'll fail if the inspection is made in another module without a public protection.

To be clear anything like that shouldn't be proposed until something like this is implemented.

If you merge this without the new behavior against protection for __trait() then you'll get a bug report, certainly. Can't say when, but this will happen.

I don't think it causes that big of a problem that it's worth waiting for a DIP that's still being written. When or even if that DIP is introduced, it won't cause any breakage to existing code and it'll be a small patch to allow private protected symbols for isStaticMember. I don't see there being very many cases where it would need to be made known whether a private member is static in another module that it won't be able to access anyways.

It's not a DIP anymore, it'll be done without following the DIP process. The question is when.

see #5112 (comment)
Doesn't look like they are going to be going with that exact DIP implementation, so changes won't be necessary.

skl131313 · 2017-02-10T23:53:30Z

Hmm not sure why @dlang-bot isn't linking the issue with bugzilla.

wilzbach · 2017-02-11T00:36:15Z

Hmm not sure why @dlang-bot isn't linking the issue with bugzilla.

You need to mention it with the git commit message.
See also: https://github.com/dlang-bots/dlang-bot#automated-references

dlang-bot · 2017-02-11T00:51:39Z

Fix	Bugzilla	Description
✗	16485	Add trait for determining whether a member variable is static or not

jmdavis · 2017-02-12T04:42:20Z

Opening this new pull request as @jmdavis seems busy and hasn't replied to pings,

Well, I didn't think that there was really anything left to do on it. The only comment on there had to do with adding "returns" to the docs, which seems inappropriate to me, since it's not a function, but whatever.

No without the required change to the protection when certain __trait() verbs are used. Which is the case here.

I don't think that this particularly matters for this PR. Yes, the language needs to be fixed, but type introspection in general is broken until the language is fixed, and worst case, using this template just results in compilation errors when you use it on something with inaccessible symbols. Yes, it sucks, but there's no fix for it without fixing the language, and it's already been agreed that we're going to fix the language. So, this template will work on some code for now and fail to compile on other code, and then later, it'll start compiling on that code too.

wilzbach · 2017-02-16T18:10:56Z

@sprinkle131313 this still needs a changelog entry. Please see the /changelog folder for instructions and examples.

skl131313 · 2017-02-16T19:13:04Z

@wilzbach
The instructions to build the changelog only leads to a command that prints that the feature will be implemented soon.

https://github.com/dlang/phobos/tree/master/changelog
https://github.com/dlang/dlang.org/blob/master/posix.mak#L523

wilzbach · 2017-02-16T19:45:53Z

The instructions to build the changelog only leads to a command that prints that the feature will be implemented soon.

Yeah nobody has merged this PR yet: dlang/dlang.org#1549
Just copy/paste another entry then, the changelog builder when run for the next release will pick it up.

andralex

Cool, please rename to hasStaticMember for symmetry with hasMember. Thanks!

andralex · 2017-02-28T18:44:55Z

std/traits.d

+
+        void m() {}
+        final void m2() const pure nothrow @nogc @safe {}
+


some of these are not covered; if they are needed as declarations but never called please don't define

jmdavis · 2017-03-01T17:04:29Z

Cool, please rename to hasStaticMember for symmetry with hasMember. Thanks!

This isn't testing whether the type has a member by that name that's static. It's testing whether a member that it definitely has is static or not. If it were going to test whether a type had a member and if it were static, then its implementation would need to be changed, and doing that would result in unnecessary redundancy, because the typical way to use this is likely to be when you have a list of members already and are checking which ones are static.

andralex · 2017-03-01T17:11:16Z

Oh, then definitely something is off. If a member known to exist is to be verified, then isStaticMember should take by alias, e.g. isStaticMember!(S.abc) as opposed to isStaticMember!(S, "abc"). In that case, it is clearly a member so the name isStatic suffices.

andralex · 2017-03-01T17:12:06Z

the typical way to use this is likely to be when you have a list of members already and are checking which ones are static

I'm not very sure this is the case, what would be some scenarios?

jmdavis · 2017-03-01T17:35:42Z

Oh, then definitely something is off. If a member known to exist is to be verified, then isStaticMember should take by alias, e.g. isStaticMember!(S.abc) as opposed to isStaticMember!(S, "abc"). In that case, it is clearly a member so the name isStatic suffices.

I believe that the reason this is the way it is is primarily because __traits(allMembers, ...) gives all of the members as strings. Presumably, it could be done with symbols, but that would result in a very different implementation and wouldn't play very well with __traits(allMembers, ...) - though in code that was already dealing with symbols, it would be easier to deal with. Both approaches have value.

I'm not very sure this is the case, what would be some scenarios?

Manu is the one who pushed for this originally, and I don't know exactly what his use case was, but he was specifically looking to test whether a member was static, not whether a type had a member with a particular name that was static.

Personally, I've run into the need for a trait like this when dealing with serialization code (e.g. serializing a struct to JSON based on its members). In that case, you're definitely dealing with a list of members and testing whether they're static rather than trying to see if a type has a specific member that is static.

I would think that most cases where you were looking to simply use a static member variable, you wouldn't care whether it was an enum or a static variable, and you could just check whether you could use the member in the context you wanted to, and if it's a static function you're looking for, __traits(isStaticFunction, ...) solves the problem easily enough. It's checking whether a specific member is a static member variable that gets hard. Introspection of a type and looking at all of its members is the only case at the moment that I can think of where you really care that a member is static as opposed to just working with the correct syntax.

The original thread (split for some reason):

http://forum.dlang.org/post/mailman.810.1475501243.2994.digitalmars-d@puremagic.com
http://forum.dlang.org/post/mailman.811.1475502086.2994.digitalmars-d@puremagic.com

andralex · 2017-03-01T17:42:02Z

I believe that the reason this is the way it is is primarily because __traits(allMembers, ...) gives all of the members as strings.

If __traits(allMembers, ...) is already used then the effort has already been done; there's no advantage in assuming the member exists already. Right now what happens if the member doesn't exist? Is the response false or a compile-time error?

@sprinkle131313 you may want to rethink the design, thanks.

jmdavis · 2017-03-01T17:53:17Z

If __traits(allMembers, ...) is already used then the effort has already been done;

What effort is already done? All you have from allMembers is a list of strings that are the names of members of the type. You know nothing about them except that they're members of that type. The hard part here is correctly testing whether a member is static or not. It's surprisingly hard to get right. And if you have the result of allMembers, then this is by far the cleanest approach. Actually getting at the symbol to test gets far more complicated.

Is the response false or a compile-time error?

It's a compiler error. The template constraint requires that the given string be a name of one of the members of the given type. If you used allMembers, then you know that the given string is the name of a member of the given type, and you're fine. If you got the name from elsewhere and need to verify that it's a member, then just use __traits(hasMember, ..) or std.traits.hasMember before using isStaticMember.

andralex · 2017-03-01T17:55:33Z

@jmdavis I'm saying the work has already been done to figure out whether it's a member, so it's difficult to justify it. I'd rather use hasStaticMember!(Foo, "bar") than hasMember!(Foo, "bar") && isStaticMember!(Foo, "bar") if there's no advantage to the latter.

jmdavis · 2017-03-01T18:03:45Z

Hmmm. So, basically, take the current implementation and move the template constraint to a static if? It would be the same test either way, so that does make sense. It is pretty weird from the standpoint of the original goal, since in that case, you already know that it's a member, but it still works, and since the template constraint was testing it anyway, there's no extra overhead for the original use case, and there's less overhead in the case where you don't know for sure that it's a member. Okay. That makes sense.

jmdavis · 2017-03-03T09:00:15Z

std/traits.d

 *           $(LREF hasElaborateDestructor)
 *           $(LREF hasIndirections)
 *           $(LREF hasMember)
+ *           $(LREF isStaticMember)


This still needs to be changed, and your most recent commit has unrelated changes mixed in.

andralex · 2017-03-03T16:52:40Z

changelog/std-traits-hasStaticMember.dd

+struct S
+{
+    static int globalVar;
+    int localVar;


global and local should be nonstatic and static

andralex · 2017-03-03T16:53:41Z

.vscode/tasks.json

+{
+    // See https://go.microsoft.com/fwlink/?LinkId=733558
+    // for the documentation about the tasks.json format
+    "version": "0.1.0",


what's with this?

My mistake, i hate that ctrl+enter commits everything even if it wasn't staged. I keep doing it by accident.

How about a new PR that adds these files to the .gitignore, s.t. you don't need to worry about it in the future?

wilzbach

Please remove the .vscode and .lib files (that's why our CI fails).
Feel free to add them to the .gitignore instead.

wilzbach · 2017-03-03T17:06:52Z

std/traits.d

+
+        inout(int) iom() inout { return 10; }
+        static inout(int) iosf(inout int x);
+


Don't define a body when you don't call it as it will lead to coverage errors ;-)

Those ones caused link errors in the test build if they aren't there.

For reference, the related error'd build:

https://circleci.com/gh/dlang/phobos/2050

generated/linux/debug/64/unittest/libphobos2-ut.so: undefined reference to `_D3std6traits18__unittestL3606_69FZ1C1gMOFZv' generated/linux/debug/64/unittest/libphobos2-ut.so: undefined reference to `_D3std6traits18__unittestL3606_69FZ1C1pMFNdZi' generated/linux/debug/64/unittest/libphobos2-ut.so: undefined reference to `_D3std6traits18__unittestL3606_69FZ1C3iomMNgFZNgi' generated/linux/debug/64/unittest/libphobos2-ut.so: undefined reference to `_D3std6traits18__unittestL3606_69FZ1C1mMFZv'

wilzbach · 2017-03-03T17:12:10Z

changelog/std-traits-hasStaticMember.dd

@@ -0,0 +1,13 @@
+Added `std.traits.hasStaticMember` to check whether a symbol is a static member of a type.


@andralex while the current changelog parser is very forgiving, we should probably make a decision on the format:

(A) title (can be multiple lines) - empty line - description

(B) title (one line) - empty line (optional) - description

(A) is implemented as dlang/tools#220
(B) is how our parser works currently.
Personally I am quite happy with closing the PR and agreeing that a title can't be more than one line (doesn't make much sense anyways).

I'll let @CyberShadow decide, thanks!

I don't see any deciding arguments one way or another.

I think enforcing that the second line is blank would be good to avoid accidental ambiguity, and would follow formatting for git commit messages.

Suggestion: start with a more restrictive policy then relax it as experience develops. The other way is much more difficult, as we've all learned the hard way :).

I think enforcing that the second line is blank would be good to avoid accidental ambiguity,
Suggestion: start with a more restrictive policy then relax it as experience develops.

@sprinkle131313 as we seem to have reached a consensus here, would you be so kind to insert an empty line after the title?

@wilzbach how do we use that nice relatively new feature of github allowing others to amend someone's pull request?

The PR author needs to opt in, which it doesn't look like they've done here.

https://wiki.dlang.org/Guidelines_for_maintainers#Write_access_to_PRs

It's enabled here:

Anyways we put this into the reminder message of the DLang-Bot:
dlang/dlang-bot#44

It's enabled here:

Oh, oops, I was looking in the wrong place.

ghost suggested changes Feb 10, 2017

View reviewed changes

skl131313 changed the title ~~Fix issue 16485: Add trait for testing if a member is static.~~ Issue 16485: Add trait for testing if a member is static. Feb 10, 2017

skl131313 changed the title ~~Issue 16485: Add trait for testing if a member is static.~~ Issue 16485 - Add trait for testing if a member is static. Feb 10, 2017

skl131313 force-pushed the is-static-member branch from 938fd8c to 8cdae1b Compare February 11, 2017 00:51

JackStouffer added the Severity:Enhancement label Feb 12, 2017

skl131313 force-pushed the is-static-member branch from 8cdae1b to da3e959 Compare February 16, 2017 19:10

skl131313 force-pushed the is-static-member branch from da3e959 to eee2785 Compare February 16, 2017 19:25

wilzbach modified the milestone: 2.074.0 Feb 17, 2017

wilzbach mentioned this pull request Feb 17, 2017

Implement issue# 16485. Add trait for testing if a member is static. #4834

Closed

andralex requested changes Feb 28, 2017

View reviewed changes

skl131313 force-pushed the is-static-member branch 3 times, most recently from f4a083c to c85814e Compare March 3, 2017 05:19

jmdavis reviewed Mar 3, 2017

View reviewed changes

andralex approved these changes Mar 3, 2017

View reviewed changes

wilzbach suggested changes Mar 3, 2017

View reviewed changes

wilzbach reviewed Mar 3, 2017

View reviewed changes

Issue 16485 - Add trait for testing if a member is static.

d79da1d

skl131313 force-pushed the is-static-member branch from e0da18e to d79da1d Compare March 4, 2017 05:12

sprinkle131313 added 2 commits March 4, 2017 00:15

Fix variable names.

4acd7ae

Add separating line in changelog.

0c1e96c

wilzbach approved these changes Mar 4, 2017

View reviewed changes

JackStouffer added the Merge:auto-merge label Mar 8, 2017

dlang-bot merged commit eb92dc9 into dlang:master Mar 8, 2017


		void m() {}
		final void m2() const pure nothrow @nogc @safe {}


		inout(int) iom() inout { return 10; }
		static inout(int) iosf(inout int x);

		@@ -0,0 +1,13 @@
		Added `std.traits.hasStaticMember` to check whether a symbol is a static member of a type.

Uh oh!

Issue 16485 - Add trait for testing if a member is static. #5112

Issue 16485 - Add trait for testing if a member is static. #5112

Uh oh!

Conversation

skl131313 commented Feb 10, 2017

Uh oh!

RazvanN7 commented Feb 10, 2017

Uh oh!

ghost left a comment • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skl131313 commented Feb 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wilzbach commented Feb 11, 2017

Uh oh!

dlang-bot commented Feb 11, 2017

Uh oh!

jmdavis commented Feb 12, 2017

Uh oh!

wilzbach commented Feb 16, 2017

Uh oh!

skl131313 commented Feb 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wilzbach commented Feb 16, 2017

Uh oh!

andralex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmdavis commented Mar 1, 2017

Uh oh!

andralex commented Mar 1, 2017

Uh oh!

andralex commented Mar 1, 2017

Uh oh!

jmdavis commented Mar 1, 2017

Uh oh!

andralex commented Mar 1, 2017

Uh oh!

jmdavis commented Mar 1, 2017

Uh oh!

andralex commented Mar 1, 2017

Uh oh!

jmdavis commented Mar 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skl131313 Mar 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost left a comment •

edited by ghost

Loading

skl131313 commented Feb 10, 2017 •

edited

Loading

skl131313 commented Feb 16, 2017 •

edited

Loading

wilzbach left a comment •

edited

Loading

skl131313 Mar 4, 2017 •

edited

Loading