Add the section to encode setters as a vec of setter interface

[Atry]

This PR is expected to be squashed when merging into main branch. The commits in this PR is not supposed to be reviewed separately.

[fredemmott]

Happy with the topic commit, but consider the comments.

Requesting changes due to previous feedback on base rev - not mergable currently.

[fredemmott]

they should be exposed as a vec

Is 'they' the C structure, or the functions?

This recommendation feels pretty specific to posix_spawn_file_actions - e.g. it doesn't feel like it would apply to anything related to sockaddr. It shouldn't replace the previous item, but it does provide a specific exception in one case.

Perhaps s/a C data structure/a C data structure that represents a list/ ?

[Atry]

The problem is that there is not a good definition of "represents a list". Technically a vec can represent an arbitrary series of function calls, as long as they are setters, where "setter" is defined as a function that just changes the data structure and does not perform global side effect.

[Atry]

Since there is no setter functions for sockaddr in the C header, I would rather not include its encoding in this section.

[fredemmott]

The problem is that there is not a good definition of "represents a list".

It doesn't need to be 100% unambiguous; refining the definition is definitely something that can be left for later - actually following anything in these documents will still require code review, so there will be human judgement later in the process.

Yes, it's possible to represent an arbitrary series of function calls, but as a general solution, it's not fitting with the idea of merely trying to map C idioms to the corresponding Hack ones.

[Atry]

Is 'they' the C structure, or the functions?

'They' include the C pointer or handle type, the constructor, the setters, and the destructor.

[Atry]

Yes, it's possible to represent an arbitrary series of function calls, but as a general solution, it's not fitting with the idea of merely trying to map C idioms to the corresponding Hack ones.

I think it's not a problem to be general here, because this section describes the default encoding for setters. We can describe other special cases to map all C idioms to Hack in other sections, especially the shape encoding for unordered setters.

[fredemmott]

I think our disagreement is that I consider the 'vec-like' thing to be the special case which should be an exception, rather than it being the general case with other things being exceptions :)

[Atry]

In fact "they should be exposed as a vec" will be modified to "they should be exposed as a vec or shape." in #175 . So the disagreement would not be a problem once we have #175

[fredemmott]

ok, the current PR + diff stack structure is a bit hard to review :) #175 still mentions 'long-lived' which I don't think is intentional.

I think this lines up with what you're writing and referential transparency, but to check: imagine 3 APIs:

C++ byref

FooStruct myFoo;
// void do_foo(FooStruct&)
do_foo(myFoo);

C pointer-to-value

FooStruct myFoo;
// void do_foo(FooStruct*)
do_foo(&myFoo);

C opaque handle

FooStruct* myFoo = foo_init();
// void do_foo(FooStruct*)
do_foo(myFoo);
foo_destroy(myFoo);

---

Assuming that all 3 only differ in terms of resource management and calling convention, these 3 approaches should be considered equivalent from the perspective of Hack API design; which approach the C library actually takes is an implementation detail. If libfoo changes between these approaches in v(X+1), we should not feel any need to change the corresponding Hack APIs.

Is this view compatible with your plans here?

[fredemmott]

Just a few small things left; once these are addressed, I'll merge, then follow up with suggetsed approachability/readability improvements.

[fredemmott]

It might be worth splitting out the first 3 commits from this stack to a separate PR: those are just nits, but I don't think we're on the same page about whether this section in the top commit is valuable

[Atry]

It might be worth splitting out the first 3 commits from this stack to a separate PR: those are just nits, but I don't think we're on the same page about whether this section in the top commit is valuable

I created the top commit because readonly public is illegal to Hack parser.

[fredemmott]

Sorry, to clarify: 1 PR with all 3 of those commits -> quick merge :)

[Atry]

Sorry, to clarify: 1 PR with all 3 of those commits -> quick merge :)

Sorry I don't understand this. Would you mind if I squash all the commits in this PR?

[fredemmott]

Can you make 1 PR which only contains the first 3 commits from this PR?

On that's in, combining/squashing this with the shapes PR might be best

[Atry]

Can you make 1 PR which only contains the first 3 commits from this PR?

Why? The first 3 commits include some problems and rest commits just fix these problems. It does not make sense to me to exclude these fixes.

[fredemmott]

similar to previous comment.

[Atry]

Which comment?

[fredemmott]

File descriptors are used as an example, but this is not the case for file descriptors.

[Atry]

I don't understand this comment, because I think file descriptors are not referential transparent C pointers, and I did not use file descriptors as an example for the definition of referential transparent C pointers.

[Atry]

Close this PR because it has been squashed into #175