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 22141 - Property .capacity is not listed in the array prope… #3356

Merged
merged 2 commits into from
Sep 28, 2022

Conversation

RazvanN7
Copy link
Contributor

…rties section

Copy link
Contributor

@thewilsonator thewilsonator left a comment

Choose a reason for hiding this comment

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

the line

This is a fixed quantity for static arrays. It is of type $(D size_t).)

ends up being repeated

@RazvanN7
Copy link
Contributor Author

@thewilsonator done. It was a copy paste error on my part.

@adamdruppe
Copy link
Contributor

Is capacity actually part of the language spec, or just a runtime library feature? It is documented in druntime's docs: http://druntime.dpldocs.info/object.capacity.html

@RazvanN7
Copy link
Contributor Author

Is capacity actually part of the language spec, or just a runtime library feature? It is documented in druntime's docs: http://druntime.dpldocs.info/object.capacity.html

I think that this distinction is rather moot. It's a property that users may use, therefore it should be present in the docs.
Since the compiler frontend specifically knows about capacity, I would say that it should be included, if that's not the case, in the language spec. Any alternative runtime should implement capacity otherwise you will end up with linker errors.

@ljmf00
Copy link
Member

ljmf00 commented Jul 29, 2022

Is capacity actually part of the language spec, or just a runtime library feature? It is documented in druntime's docs: http://druntime.dpldocs.info/object.capacity.html

I think that this distinction is rather moot. It's a property that users may use, therefore it should be present in the docs. Since the compiler frontend specifically knows about capacity, I would say that it should be included, if that's not the case, in the language spec. Any alternative runtime should implement capacity otherwise you will end up with linker errors.

I agree. It's not hard to dummy the capacity to be the same as length, in case some runtime doesn't support querying the BlkInfo, so I would say we should enforce this to exist to increase portability.

@adamdruppe
Copy link
Contributor

It's a property that users may use, therefore it should be present in the docs.

It is present in the docs.

Since the compiler frontend specifically knows about capacity,

Where? I just grepped it and the only time I see it appearing is

    counter += l.capacity; // Make sure l is not optimized out.

in a test file, where it is just used as a hack to the optimizer instead of any actual spec test.

There are no references to the _d_arraysetcapacity implementation

Any alternative runtime should implement capacity otherwise you will end up with linker errors.

It would be compile errors, not linker errors, if and only if the function is actually called. Which is the same as any other function or module in there.

What about assumeSafeAppend? Or core.thread and core.sync which are mentioned as "built-in" features elsewhere in the spec (see the betterC page for example)?

.dup and .idup used to be built in, just like .ptr and .length. But they aren't anymore either. So really, why are there some druntime functions listed here and other ones not? It seems completely arbitrary.

BTW speaking of betterC (which is terrible and should be removed), if this PR is merged, dmd -betterC would no longer be compliant with the spec. Which is fine by me, again, it is terrible and should be removed, but you're introducing an inconsistency.

void main() {
        int[5] s;
        assert(s.capacity == 0);
}

$ dmd sppp -betterC
/home/me/d/dmd2/linux/bin64/../../src/druntime/import/object.d(3661): Error: `TypeInfo` cannot be used with -betterC

@RazvanN7
Copy link
Contributor Author

RazvanN7 commented Jul 29, 2022

Where? I just grepped it and the only time I see it appearing is

Wow, you are right. I just looked at the dmd code and it seems that the compiler simply tries UFCS for a a.b expression if b is not a field/property of a. Since capacity is a template defined in druntime it just works. I find this very brittle. I assumed that since it worked, the compiler specifically recognizes capacity and then lowers to a call to druntime.

I see your point now, capacity is not an array property, but rather a convenience function that is implemented in druntime. Still, from a user perspective, capacity looks like an array property. What happens behind the scenes (a hack, if you ask me) is not relevant in this case.

What about assumeSafeAppend? Or core.thread and core.sync which are mentioned as "built-in" features elsewhere in the spec (see the betterC page for example)?

I don't see the relevance of this example. We are talking about the user perspective here, not how the compiler devs categorize the myriad of functions that are being called implicitly by the compiler.

.dup and .idup used to be built in, just like .ptr and .length. But they aren't anymore either. So really, why are there some druntime functions listed here and other ones not? It seems completely arbitrary.

Because you can directly call them as if they were properties of an array without any intervention whatsoever. In my opinion, if you can do array.whatever without needing to import anything, then whatever should be in the list of properties for arrays, regardless of how the compiler actually resolves the symbol.

@adamdruppe
Copy link
Contributor

(a hack, if you ask me)

.sort used to be a built-in property too. Then it got replaced by a UFCS function. This "hack" is the direction the language has been going for some time.

I don't see the relevance of this example.

assumeSafeAppend is one way how you explicitly modify capacity (another one being reserve). It is another function in object. From the user perspective, capacity, reserve, and assumeSafeAppend all work exactly the same way.

As you yourself said: "if you can do array.whatever without needing to import anything, then whatever should be in the list of properties for arrays"

(I disagree with that statement, but my point is that your position is inconsistent with itself.)

Because you can directly call them as if they were properties of an array without any intervention whatsoever.

They're exactly the same as any other library import, including the possibility of overrides and conflicts:

module cap2;
void idup(T)(T t) {}
import cap2;
void main() {
        int[] a;
        a.idup;
}

cap.d(4): Error: function cap2.idup!(int[]).idup at cap2.d(2) conflicts with
unction object.idup!int.idup at /home/me/d/dmd2/linux/bin64/../../src/druntime/import/object.d(3555)

That does NOT happen with the fully built ins: try changing idup to length there and find no error. length is built into the compiler, idup isn't.

So, what's the actual logic here in terms of the spec?

@ntrel
Copy link
Contributor

ntrel commented Jul 30, 2022

@adamdruppe sort, reserve and assumeSafeAppend modify the array. capacity does not, it's just a getter property similar to length. Fair point about built in properties vs runtime functions and overloading.
(Unlike sort, I think capacity is something only the runtime can calculate. It couldn't be implemented outside the runtime without knowledge of the internal runtime array code not in a public API.)

spec/arrays.dd Outdated Show resolved Hide resolved
@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @RazvanN7! 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
22141 enhancement Property .capacity is not listed in the array properties section

Co-authored-by: Nick Treleaven <ntrel002@gmail.com>
@dkorpel dkorpel merged commit 2c0d274 into dlang:master Sep 28, 2022
@PetarKirov
Copy link
Member

PetarKirov commented Oct 5, 2022

I think that a better litmus test for whether the language spec should specify that $fun is a built-in property of arrays (or any other type), as opposed to only listing $fun in the library docs, is to imagine a completely independent implementation of the language and to ask the question: what is the minimum amount of features that they must implement, for their implementation to be considered compliant with the D language spec? And specifically, whether $func is part of this minimal set of features.

Now, an argument can be made about Druntime and Phobos: are competing implementations of the language supposed to be able to reuse Druntime and Phobos, with minimum to no changes from the reference implementation? Perhaps only Phobos? But what about core.sys - surely that's so simple that any compliant implementation of language should be able to reuse it? And if you look at the other stuff in core.*, most (all?) of them look like regular D code that any user of the language should expect to be able to write for their project.

TL;DR Instead of arguing about is an isn't part of the language based on the current implementation, we should decide what is the minimum set of stuff that is supposed to be. Otherwise, there will be always push and pull about whether we should document the implementation or change it to match the docs.

@RazvanN7
Copy link
Contributor Author

RazvanN7 commented Oct 5, 2022

Honestly, I find the distinction irrelevant. The reader of the docs is 99.9% of times someone who doesn't care about the implementation. For that particular person, the fact that you can use .capacity without importing anything makes it look like it is a builtin property. The fact that .capacity is implemented as a template in druntime is an implementation detail that nobody actually cares about so it could be documented as a property because it looks like one.

If we want to highlight the distinction between builtin properties and druntime calls we can do that, but that brings exactly 0 value to anyone reading the docs.

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.

8 participants