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 12954 - Deprecated only works with string literals #5302

Merged
merged 1 commit into from
Feb 10, 2016

Conversation

Geod24
Copy link
Member

@Geod24 Geod24 commented Dec 6, 2015

https://issues.dlang.org/show_bug.cgi?id=12954

Allow code to use enum and static {immutable,const} for the deprecation message.
This will allow for more dynamic deprecation message (e.g. with release informations).
It purposedly doesn't support direct function call to get the deprecation message.

@dlang-bot
Copy link
Contributor

Fix Bugzilla Description
12954 deprecated doesn't work with concatenated strings or anything else but a string literal

@@ -411,6 +411,11 @@ public:
{
assert(msg);
char* depmsg = null;
sc = sc.startCTFE();
this.msg = this.msg.semantic(sc);
Copy link
Member

Choose a reason for hiding this comment

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

Prefixing with this is not necessary.

@Geod24 Geod24 force-pushed the fix-12954 branch 5 times, most recently from 714847e to 98ac415 Compare January 31, 2016 17:13
@Geod24
Copy link
Member Author

Geod24 commented Jan 31, 2016

Updated

}

override void semantic2(Scope* sc)
{
assert(msg);
char* depmsg = null;
Copy link
Member

Choose a reason for hiding this comment

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

Dead code?

Copy link
Member Author

Choose a reason for hiding this comment

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

RIchtig ! Fixed.

@yebblies
Copy link
Member

yebblies commented Feb 1, 2016

This looks ok to me. @9rnsr ?

msg = msg.semantic(sc);
msg = resolveProperties(sc, msg);
sc = sc.endCTFE();
msg = msg.optimize(WANTexpand);
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to use msg.ctfeInterpret() instead.

@Geod24
Copy link
Member Author

Geod24 commented Feb 8, 2016

Updated according to @9rnsr 's comments:

  • s/message/msgstr/
  • msg.optimize(WANTexpand) -> msg.ctfeInterpret()
  • Changed setScope for newScope.

Also documented the functions and added a small test with a deprecated class + deprecated member to reflect the new implementation.

@9rnsr : StorageClassDeclaration.newScope was final, so I had to remove this annotation. Not sure how good that is though.

@9rnsr
Copy link
Contributor

9rnsr commented Feb 8, 2016

Excellent work!

StorageClassDeclaration.newScope was final, so I had to remove this annotation. Not sure how good that is though.

Of course there's no problem. The final was there just because there was no derived class who overrides StorageClassDeclaration.newScope method. Removing it is legitimate change.

@9rnsr
Copy link
Contributor

9rnsr commented Feb 8, 2016

I'll get this enhancement change. @Geod24, can you also write a changelog section for the "Language Enhancement"? In #5425, I'm resetting changelog.dd. After it's merged, let's rebase this PR on master, with the new changelog text.

@Geod24
Copy link
Member Author

Geod24 commented Feb 8, 2016

@Geod24, can you also write a changelog section for the "Language Enhancement"?

Will do once #5425 is merged. I'll also prepare a P.R. to dlang.org documenting the grammar change.

@9rnsr
Copy link
Contributor

9rnsr commented Feb 9, 2016

#5425 is merged.

@Geod24
Copy link
Member Author

Geod24 commented Feb 10, 2016

Rebased on master, changelog entry added.

Allow code to use `enum` and `static {immutable,const}` for the deprecation message.
This will allow for more dynamic deprecation message (e.g. with release informations).
It purposedly doesn't support direct function call to get the deprecation message.
@9rnsr
Copy link
Contributor

9rnsr commented Feb 10, 2016

Thanks!

@9rnsr
Copy link
Contributor

9rnsr commented Feb 10, 2016

Auto-merge toggled on

@Geod24
Copy link
Member Author

Geod24 commented Feb 10, 2016

Dlang.org P.R.: dlang/dlang.org#1229

9rnsr added a commit that referenced this pull request Feb 10, 2016
Fix issue 12954 - Deprecated only works with string literals
@9rnsr 9rnsr merged commit 96b883a into dlang:master Feb 10, 2016
@Geod24 Geod24 deleted the fix-12954 branch February 10, 2016 12:13
@9rnsr
Copy link
Contributor

9rnsr commented Feb 11, 2016

Mm, I had overlooked this statement written in the summary.

It purposedly doesn't support direct function call to get the deprecation message.

While the reviewing I've proposed this change.

msg.optimize(WANTexpand) -> msg.ctfeInterpret()

By that, now any compile time function calls for the custom deprecation message is properly allowed...

@Geod24 , why you've thought that function call use has to be rejected as a custom message? I'd just hear your argument.

@Geod24
Copy link
Member Author

Geod24 commented Feb 11, 2016

@Geod24 , why you've thought that function call use has to be rejected as a custom message? I'd just hear your argument.

I'm very happy with it accepting direct function call. I think it was originally rejected because I was afraid it would cause forward references, but that was down to using setScope as you pointed.
Will update the dlang.org P.R. accordingly.

@9rnsr
Copy link
Contributor

9rnsr commented Feb 11, 2016

I understand, thanks.

Geod24 added a commit to Geod24/dlang.org that referenced this pull request Feb 12, 2016
Geod24 added a commit to Geod24/dlang.org that referenced this pull request Feb 12, 2016
@MartinNowak
Copy link
Member

Seems like this introduced a small regression.
Issue 15815 – [Reg 2.071-devel] deprecation for aliased template instance not shown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants