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 20915 - add support for is(identifier == version) #11255

Closed
wants to merge 1 commit into from
Closed

fix issue 20915 - add support for is(identifier == version) #11255

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Jun 10, 2020

This fills the small gap that existed because __traits(allMembers) could return the identifiers of the VersionSymbols defined in the source but there was no way to identify them as such.

If accepted spec PR will follow.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @NilsLankila! 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 coverage diff by visiting the details link of the codecov check)
  • 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
20915 enhancement __traits(allMembers) results include custom version identifiers, which is unusable

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

@Geod24
Copy link
Member

Geod24 commented Jun 10, 2020

Hum... Should allMembers really return locally-defined versions ? Is there any use case for it ?

Copy link
Member

@ibuclaw ibuclaw left a comment

Choose a reason for hiding this comment

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

Bar the small implementation note. Please read up on the last 20 years history of why version (!ident) has been rejected time and time again.

src/dmd/expressionsem.d Outdated Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Jun 10, 2020

Please read up on the last 20 years history of why version (!ident) has been rejected time and time again.

I'm aware of that but this is not an attempt to introduce fragments of version expressions sneakily.
Any attempt would clearly be visible in the parser.

@ghost ghost requested a review from ibuclaw June 10, 2020 11:55
This fills the small gap that existed because `__traits(allMembers)` could return the identifiers of the `VersionSymbol`s defined in the source but there was no way to identify them as such.
@ibuclaw
Copy link
Member

ibuclaw commented Jun 10, 2020

I'm aware of that but this is not an attempt to introduce fragments of version expressions sneakily.
Any attempt would clearly be visible in the parser.

But what arguable difference would there be between the rejected version (linux || OSX) versus static if (is(linux == version) || is(OSX == version))? They are functionally identical.

@ghost
Copy link
Author

ghost commented Jun 10, 2020

indeed, incredible, I didn't realize at all that the change allowed version operators, although the syntax is a bit verbose.

@ghost
Copy link

ghost commented Jun 10, 2020

But what arguable difference would there be between the rejected version (linux || OSX) versus static if (is(linux == version) || is(OSX == version))? They are functionally identical.

People already abuse 'enum + static if' everywhere to work around the absence of version expressions. If allMembers returns versions, there must exist a predicate to filter them out.

@ibuclaw
Copy link
Member

ibuclaw commented Jun 10, 2020

indeed, incredible, I didn't realize at all that the change allowed version operators, although the syntax is a bit verbose.

Well I'd probably let @WalterBright weigh in, but my feeling is that you should not be including predefined symbols in the is(ident == version) match.

What about @Geod24's question? Is there a use case for allMembers returning versions?

@WalterBright
Copy link
Member

Various versions of this proposal (algebraic combinations of versions) have come up before. We are not doing this. The version system in D is specifically designed to discourage this sort of programming. Besides, if you want to know if a version is a version:

version (isthisaversion)
    pragma(msg, "isthisaversion is indeed a version");
else
    pragma(msg, "never heard of isthisaversion");

@WalterBright
Copy link
Member

__traits(allMembers) must not return version ids or debug ids.

@ghost ghost deleted the isexpr-version branch June 10, 2020 23:11
@adamdruppe
Copy link
Contributor

I've actually been wanting some kind of available version detection to help document possible compile time options.

I'll probably work it into my doc generator though since it has its own parser and can look for both version=X and version(X) and at least generate a list.

@ghost
Copy link

ghost commented Jun 11, 2020

Various versions of this proposal (algebraic combinations of versions) have come up before. We are not doing this. The version system in D is specifically designed to discourage this sort of programming. Besides, if you want to know if a version is a version:

version (isthisaversion)
    pragma(msg, "isthisaversion is indeed a version");
else
    pragma(msg, "never heard of isthisaversion");

O, gosh! This was not a proposal for algebraic combinations at all! We are well aware of your aversion to those.

Having an ability to introspect into version identifiers is a completely different and potentially useful feature (Adam has a use for it). The fact that the version-detecting predicate can be abused is completely irrelevant (as I said, people are already abusing 'static if' to work around your ban on version algebra).

Side note: language design based mainly on preexisting use cases and personal experience means another dead language in the near future.

@ghost
Copy link
Author

ghost commented Jun 11, 2020

there's a callback_API now in DMD if you need things that don't change the semantic...

@ghost
Copy link
Author

ghost commented Jun 11, 2020

@adamdruppe see 2efa524. You can propose more callbacks if you need.

@ghost
Copy link
Author

ghost commented Jun 11, 2020

@maxsamukha, it wasn't sure, not directly ;]

@WalterBright
Copy link
Member

language design based mainly on preexisting use cases and personal experience means another dead language in the near future.

Indeed I do have 40 years of experience with the stinky pile of poo that always results from algebraic combinations of versions.

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.

6 participants