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

Allow const declarations on mutable fields #43305

Merged
merged 1 commit into from
Dec 17, 2021
Merged

Allow const declarations on mutable fields #43305

merged 1 commit into from
Dec 17, 2021

Conversation

vtjnash
Copy link
Sponsor Member

@vtjnash vtjnash commented Dec 2, 2021

Mark some builtin types also.

Replaces #11430

Co-authored-by: Matt Bauman mbauman@gmail.com

@vtjnash vtjnash requested a review from mbauman December 2, 2021 18:35
NEWS.md Outdated Show resolved Hide resolved
@mbauman mbauman removed their request for review December 2, 2021 21:22
@mbauman
Copy link
Sponsor Member

mbauman commented Dec 2, 2021

This is really cool, but removing myself from the review since I neither have the bandwidth nor expertise to meaningfully review this anymore.

@fredrikekre
Copy link
Member

Is it more common to have a few mutable fields instead of a few const fields? Maybe it could make sense to have e.g.

struct ABC
    mutable a::Int
    b::Int
    c::Int
end

instead of having to do

mutable struct ABC
    a::Int
    const b::Int
    const c::Int
end

? Or perhaps support both...

base/compiler/tfuncs.jl Outdated Show resolved Hide resolved
base/compiler/tfuncs.jl Outdated Show resolved Hide resolved
@jtrakk
Copy link

jtrakk commented Dec 6, 2021

mutable storage class specifier is a related C++ feature:

The mutable storage class specifier is used only on a class data member to make it modifiable even though the member is part of an object declared as const.

class A
{
  public:
    A() : x(4), y(5) { };
    mutable int x;
    int y;
};

int main()
{
  const A var2;
  var2.x = 345;
  // var2.y = 2345;
}

@KristofferC
Copy link
Sponsor Member

Maybe it could make sense to have e.g....

Not sure, a mutable struct with some fields marked const is still mutable, while the opposite is not true (immutable struct with mutable field is no longer immutable) so to me it makes more sense to have the annotation be a restriction.

@rfourquet
Copy link
Member

a mutable struct with some fields marked const is still mutable, while the opposite is not true (immutable struct with mutable field is no longer immutable)

While I kinda agree (we are used to interpret struct X ... as immutable X...), a counterpoint is that struct is not spelled immutable, so the base for defining a "struct" is struct X end, and mutable can be prepended to make the whole struct mutable, i.e. all the fields, and with the proposal it could alternatively be applied to only specific fields.

@jtrakk
Copy link

jtrakk commented Dec 6, 2021

In that case would these two be equivalent?

mutable struct Foo
a
b
end

mutable struct Foo
mutable a
mutable b
end

Mark some builtin types also, although Serialization relies upon being
able to mutilate the Method objects, so we do not yet mark those.

Replaces #11430

Co-authored-by: Matt Bauman <mbauman@gmail.com>
@JeffBezanson
Copy link
Sponsor Member

I agree with Kristoffer that adding a mutable field to a struct is a more dramatic change than restricting a field of a mutable struct to constant. The problem is that mutable struct has more memory layout implications than just whether you are allowed to mutate it. To make one field of a struct mutable, you have to either change the whole thing to act like a mutable struct, or allocate a Ref cell for that one field to maintain the identity of the mutable location. In contrast, if you start with a mutable struct nothing special is needed to make one field const; you can just disallow mutating it. So I think this version of the change makes more sense.

@vtjnash vtjnash merged commit 63f6294 into master Dec 17, 2021
@vtjnash vtjnash deleted the jn/constfields branch December 17, 2021 21:59
@vchuravy vchuravy added the needs docs Documentation for this change is required label Dec 17, 2021
N5N3 added a commit to N5N3/julia that referenced this pull request Dec 28, 2021
N5N3 added a commit to N5N3/julia that referenced this pull request Dec 29, 2021
N5N3 added a commit to N5N3/julia that referenced this pull request Dec 30, 2021
N5N3 added a commit to N5N3/julia that referenced this pull request Dec 30, 2021
N5N3 added a commit to N5N3/julia that referenced this pull request Dec 31, 2021
N5N3 added a commit to N5N3/julia that referenced this pull request Jan 2, 2022
N5N3 added a commit to N5N3/julia that referenced this pull request Jan 3, 2022
N5N3 added a commit to N5N3/julia that referenced this pull request Jan 5, 2022
N5N3 added a commit to N5N3/julia that referenced this pull request Jan 6, 2022
This reverts commit 63f6294.

Revert "Make BitSet field const (JuliaLang#43553)"

This reverts commit 98b485e.
N5N3 added a commit to N5N3/julia that referenced this pull request Jan 6, 2022
N5N3 added a commit to N5N3/julia that referenced this pull request Jan 6, 2022
N5N3 added a commit to N5N3/julia that referenced this pull request Jan 7, 2022
N5N3 added a commit to N5N3/julia that referenced this pull request Jan 7, 2022
N5N3 added a commit to N5N3/julia that referenced this pull request Jan 8, 2022
@vtjnash vtjnash removed the needs docs Documentation for this change is required label Jan 12, 2022
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
Mark some builtin types also, although Serialization relies upon being
able to mutilate the Method objects, so we do not yet mark those.

Replaces JuliaLang#11430

Co-authored-by: Matt Bauman <mbauman@gmail.com>
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
Mark some builtin types also, although Serialization relies upon being
able to mutilate the Method objects, so we do not yet mark those.

Replaces JuliaLang#11430

Co-authored-by: Matt Bauman <mbauman@gmail.com>
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.

None yet

10 participants