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.

Sign up for GitHub

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

Jump to bottom

Fix compiler warnings and deprecations emitted by latest dmd #10

Merged
merged 4 commits into from
Mar 30, 2023
Merged

Fix compiler warnings and deprecations emitted by latest dmd #10

merged 4 commits into from
Mar 30, 2023

Conversation

cschlote
Copy link
Contributor

The latest DMD emits now warnings for

  • deprecations on scoped data (possibly compiler bugs?)
  • use of nothrow on method with in contract using assert

This PR proposes some possible solutions for these warnings and deprecations..

@codecov-commenter
Copy link

codecov-commenter commented Nov 26, 2022 •
edited
Loading

Codecov Report

Merging #10 (78a821c) into master (b4ed612) will not change coverage.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##           master      #10   +/-   ##
=======================================
  Coverage   92.88%   92.88%           
=======================================
  Files           1        1           
  Lines         478      478           
=======================================
  Hits          444      444           
  Misses         34       34
Flag Coverage Δ
unittests 92.88% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
source/semver.d 92.88% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@cschlote cschlote force-pushed the fix-compiler-warnings-and-deprecations-emitted-by-latest-dmd branch from 41848be to 78a821c Compare November 26, 2022 17:00
Geod24
Geod24 reviewed Nov 28, 2022
View reviewed changes
source/semver.d
if (!build.empty)
semVer ~= "+" ~ "%-(%s.%)".format(build);
semVer ~= "+" ~ "%-(%s.%)".format(cast(const char[]) build);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, with the error message I see the issue. std.format is not accepting its parameter by scope.
So casting the scope away is indeed the "best" solution here, because scope on toString is the right thing. The problem is in std.format.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. So format() needs to be changed as the real fix?

Copy link
Collaborator

@Geod24 Geod24 Nov 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe. That's where DIP1000 shows its limitations. If you change format to accept its parameter by scope, you will break users that have non-scope toString implementations. That includes classes. Oops.

Geod24
Geod24 reviewed Nov 28, 2022
View reviewed changes
source/semver.d Outdated
@@ -185,7 +185,7 @@ struct SemVer
return result;
}

private SemVer appendPrerelease0() scope @safe pure nothrow
private SemVer appendPrerelease0() @safe pure nothrow
Copy link
Collaborator

Choose a reason for hiding this comment

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

So, in short:

  • scope as function attribute means that the function may be called with a scope parameter;
  • return scope allows you to call it with a scope parameter and returns a reference that is linked to this;

Hence why I suggested return scope. What does the compiler say ?

Geod24
Geod24 reviewed Nov 28, 2022
View reviewed changes
source/semver.d
@@ -372,7 +372,7 @@ struct SemVer
/**
* Compare two $(B different) versions and return the parte they differ on.
*/
VersionPart differAt(ref const SemVer other) const scope @safe pure nothrow
VersionPart differAt(ref const SemVer other) const scope @safe pure
Copy link
Collaborator

Choose a reason for hiding this comment

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

With the error message, it's obvious what is wrong: assert is nothrow, because throwing an Error is nothrow, but this != other calls opEquals which I am pretty sure is not nothrow.

@Geod24
Copy link
Collaborator

Geod24 commented Nov 28, 2022

Thanks for taking care of this!

@cschlote
Workaround for depractation warning for scope used with toString() me…
76859ea 
…thod

Latest DMD beta reports:
source/semver.d(144,47): Deprecation: scope variable `this` assigned to non-scope parameter `_param_1` calling `format`
source/semver.d(146,47): Deprecation: scope variable `this` assigned to non-scope parameter `_param_1` calling `format`

Can be fixed by removing 'scope' or by adding two casts.

The problem seems to be related to format() not accepting 'scope' parameters. Casting away the 'scope' removes the problem.
@cschlote
Use 'return scope' for appendPrerelease0()
4c43cd4 
Latest DMD reports:
source/semver.d(192,16): Deprecation: scope parameter `this` may not be returned

Can be fixed by using 'return scope'.
@cschlote
Add scope to suffix parameter of compareSuffix
db84851 
Latest DMD reports:
source/semver.d(342,57): Deprecation: scope variable `this` assigned to non-scope parameter `suffix` calling `compareSufix`
source/semver.d(346,54): Deprecation: scope variable `this` assigned to non-scope parameter `suffix` calling `compareSufix`

Can be reasonably fixed by adding scope to suffix parameter.
@cschlote
Removed 'nothrow' from differAt() method, because of in contract
7f6fa19 
Latest DMD reports:
source/semver.d(375,17): Deprecation: `semver.SemVer.differAt`: `in` contract may throw but function is marked as `nothrow`

Can be reasonably fixed by removing the in{} contract with the assert(),
or by removing 'nothrow' from these methods.
@cschlote cschlote force-pushed the fix-compiler-warnings-and-deprecations-emitted-by-latest-dmd branch from 78a821c to 7f6fa19 Compare November 28, 2022 15:04
@Geod24
Copy link
Collaborator

Geod24 commented Nov 28, 2022

Note: The correct fix for the last commit is to make opEquals / opCmp nothrow.

@cschlote
Copy link
Contributor Author

I tried to make opEquals / opCmp nothrow, but htis results into:

source/semver.d(342,35): Error: function `semver.SemVer.opCmp.compareSufix` is not `nothrow`
source/semver.d(346,32): Error: function `semver.SemVer.opCmp.compareSufix` is not `nothrow`
source/semver.d(300,9): Error: function `semver.SemVer.opCmp` may throw but is marked as `nothrow`
/home2/cschlote/build/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/algorithm/sorting.d(1953,9): Error: static assert:  "Invalid predicate passed to sort: "a > b""
source/semver.d(750,25):        instantiated from here: `sort!("a > b", SwapStrategy.unstable, SemVer[])`
Error dmd-beta failed with exit code 1.

and adding nothrow to semver.SemVer.opCmp.compareSufix makes things even worse:

source/semver.d(328,26): Error: function `std.conv.to!uint.to!string.to` is not `nothrow`
source/semver.d(328,39): Error: function `std.conv.to!uint.to!string.to` is not `nothrow`
source/semver.d(329,33): Error: function `std.conv.to!uint.to!string.to` is not `nothrow`
source/semver.d(329,45): Error: function `std.conv.to!uint.to!string.to` is not `nothrow`
source/semver.d(314,13): Error: function `semver.SemVer.opCmp.compareSufix` may throw but is marked as `nothrow`
/home2/cschlote/build/dlang/dmd/generated/linux/release/64/../../../../../phobos/std/algorithm/sorting.d(1953,9): Error: static assert:  "Invalid predicate passed to sort: "a > b""
source/semver.d(750,25):        instantiated from here: `sort!("a > b", SwapStrategy.unstable, SemVer[])`
Error dmd-beta failed with exit code 1.

@cschlote
Copy link
Contributor Author

$ dmd-beta --version
DMD64 D Compiler v2.103.0-rc.1-87-g7e84fb3333

Der wording of the deprecation messages changed changed slightly for 'master' branch. With the changes in this PR all compilers (gdc, ldc2, dmd & dmd-beta) compile without any warnings or deprecations.

Any open issues? Any other ways to fix compilation?

@Geod24 Geod24 merged commit a528463 into dcarp:master Mar 30, 2023
@cschlote cschlote deleted the fix-compiler-warnings-and-deprecations-emitted-by-latest-dmd branch March 30, 2023 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Geod24 Geod24 Geod24 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cschlote @codecov-commenter @Geod24