torch_list_get_size?

I'm following the semantic where if it's blah->api(), I'm naming it torch_blah_api, so this one stays.

Hmm, on one hand I understand the desire to keep error_t foo(inp, *out) semantic, but on the other hand, the only it it could fail if list_handle is nullptr, isn't it? Should we indeed for the sake of speed change semantic a bit to size_t torch_list_get_size(StableListHandle)?

I'm leaning towards no for now, because it would mean we should guarantee in the long term that the function should never fail, and that's not worth the small speed bump to me.

Hmm, on one hand I understand the desire to keep error_t foo(inp, *out) semantic, but on the other hand, the only it it could fail if list_handle is nullptr, isn't it? Should we indeed for the sake of speed change semantic a bit to size_t torch_list_get_size(StableListHandle)?

Same as above: if at::List::operator[] doesn't do boundary checks, this API shouldn't do them either

Just curious, what are the purpose of those?

These are scaffolding so that in the unlikely case that the memory layout of a struct passed as a stableivalue changes between two releases, we can appropriately modify from and to (and from_ivalue + to_ivalue that call into these) to handle this. See https://github.com/pytorch/pytorch/pull/165284/files#diff-e260bc7cf1dcf33d3ce5fa484f918553929e57bc8ae76d398d4b1d884fe8654eR420-R437 for an example use

(I'll have better developer documentation to come, there are also tests that ensure the memory layout of the relevant structs don't change coming)

Why can't we use irange for stable ABI?

We'd have to move irange to headeronly first, but otherwise we can

Do you want to check first that list_handle and size are not nullptrs?

Ehhhh these are meant to be used mostly internal so I'm not convinced we need to add checks

OK, then why do you want to wrap this code with AOTI_TORCH_CONVERT_EXCEPTION_TO_ERROR_CODE? Semantically this code will never throw, would it?

FWIW, it's harmless if size() gets inlined and so I am weakly in favor of it because it makes everything visually uniform (lack of AOTI_TORCH_CONVERT_EXCEPTION_TO_ERROR_CODE is known-bad). https://godbolt.org/z/zbqbhez87

I see your point; I have just learned that dereferencing a bad pointer will lead to crash/UB but not throw an Exception. Regardless, the way I'm thinking of adding these APIs is to be defensive against future code modifications. I would feel more comfortable removing the macro once we have a test to ensure that all shim APIs never throw Exceptions.

I indeed will land the PR as is but this is a good consideration. Whatever policy we adopt I would want to apply across all APIs --> if we check for nullptr here we should check in the Tensor shims too.

is it permissible to be wrong about the reserved size?

I think reserving size is just a performance optimization, it doesn't actually populate the tensor. So after reserving the size is still 0.

you're going to leak memory for the list and for previously-pushed elements if this TORCH_ERROR_CODE_CHECK ever fails, courtesy of StableIValue not having a destructor

ditto leaky

don't you need to catch all flavors of exception? why is it ok to leak on things that aren't runtime_error?

The only thing that TORCH_ERROR_CODE_CHECK can throw is std::runtime_error

fair enough, nothing should get thrown through ABI boundary

drop the e, just throw;. it's cleaner.

Wait does that do the same thing? Does that rethrow the error?

Ohhh it issss

generally rethrowing can also do stuff like cause backtraces to be preserved.

throw e; can cause object slicing, while throw preserve type of original exception?

same as above

OK, then why do you want to wrap this code with AOTI_TORCH_CONVERT_EXCEPTION_TO_ERROR_CODE? Semantically this code will never throw, would it?