Locations info continued + handle mod cleanup + downcasts

[aldanor]

This PR aims to finish the work on locations, location info etc. One reason is that times / num attrs would never be filled out - which needs to be handled.

[aldanor]

@mulimoen Interesting, I can't figure out why the tests fail (on 1.10.6) for datasets but not groups? I.e., you create a group and check its location info and times are filled out, but not for datasets. Am I missing something?

[aldanor]

LocationInfo {
    fileno: 71,
    token: LocationToken(
        195,
    ),
    loc_type: Group,
    refcount: 1,
    atime: 1634203887,
    mtime: 1634203887,
    ctime: 1634203887,
    btime: 1634203887,
    num_attrs: 0,
}
dataset LocationInfo {
    fileno: 71,
    token: LocationToken(
        342,
    ),
    loc_type: Dataset,
    refcount: 1,
    atime: 0,
    mtime: 0,
    ctime: 0,
    btime: 0,
    num_attrs: 2,
}

[mulimoen]

Do you mot need to turn time tracking on when creating the dataset?

[aldanor]

Needs a bit of version juggling (doing it now), changelog update and maybe a few docstrings, but almost finished all in all.

(Aside from loc info, contains a few todos I've had for a while - clean up Handle, don't re-export free functions, and add downcast methods - which is now more relevant since we can open stuff by token).

[mulimoen]

I feel we should keep and use the free-standing functions here instead of creating an invalid handle. Forgetting to forget the handle could invalidate other unrelated objects and be very hard to track down. If one panics in is_valid_user_id one could also be in the same position.

[mulimoen]

This could happen if Mutex is poisoned, although the state of this library would be in a poor shape if that was to happen

[aldanor]

The main point, I think, there's literally few lines of code where this is needed - and they would continue to work largely the same with the change. I.e., the compiled code will be pretty much identical; the only difference is that now ObjectClass::from_id checks for handle validitity first and then the id type match - which is more correct way of doing it anyways. So, we contain all handle-like low-level stuff in Handle and enclose it completely, without any free-standing functions etc. I would argue that for a newcoming contributor, too, this would be much easier to read since it's not obvious where those free-standing functions belong to and why are they pub-re-exported.

// Also, if is_valid_user_id panics, it's a panic, and after any panic all promises are broken anyway? (and it's almost surely a straight crash regardless). And you're right, with a poisoned mutex all the things around are mostly already dead anyway.

[mulimoen]

We should add a comment above each forget with

// Drop on invalid handle could cause close of unrelated object

to clearly document why this is needed.

[mulimoen]

Could the free-standing functions be moved to this scope and not exported? It is feasible to guarantee a Handle always being valid if we make decref() pub(crate)

[aldanor]

Could the free-standing functions be moved to this scope and not exported? It is feasible to guarantee a Handle always being valid if we make decref() pub(crate)

Heh - I was suggesting something similar just in a previous PR - we can make both incref/decref pub(crate) and then there's no need for free-standing functions, Handle is guaranteed to be valid, and no checks are needed in incref/decref.

[aldanor]

(Handle::invalid() would also have to be killed then)

[mulimoen]

I'll open a PR after this for invalid handles

[aldanor]

Another valid case to mention that doesn't involve manual decref - File::close(). However, it takes self by value, so it should be all good...

[mulimoen]

File::close needs a better comment, it must be closed with H5Fclose because of the file close degree