Add astGetKeyMap protected method to Object

[embray]

Resolves #54. My motivation here is primarily for use in yamlchan.c to make round-tripping certain transforms easier.

Currently it uses a lot of structural pattern-matching (e.g. FindSphericalCartesian) to reconstruct the appropriate ASDF transforms from a slice of the mapping list. This code is still useful and necessary -- e.g. for hand-constructed astMapping, or one read from another source, one will still need to figure out the most appropriate way to serialize it back to ASDF.

But for the case of deserializing from ASDF it will also be very useful to maintain additional information on the deserialized mappings for accurate round-tripping without having to go down the full Find<Foo> path. This idea is already used, to an extent, by the KeyMap attached to some mappings via astSetProxy, but that proves not as useful since the proxy object can't survive copying, per the discussion in #54. So this would be used to replace that.

Had started out with trying to reorganize ast_tester/testobject.c in anticipation of adding more unit tests for this, but then realized since it's a protected interface it needs to be compiled with astCLASS defined, so ended up having to put the tests for this in a separate test program anyways.

[codecov]

Codecov Report

❌ Patch coverage is 77.77778% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.17%. Comparing base (3a3c3e5) to head (ddc5125).

Files with missing lines	Patch %	Lines
src/object.c	77.77%	3 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #55      +/-   ##
==========================================
+ Coverage   61.13%   61.17%   +0.04%     
==========================================
  Files          83       83              
  Lines       96370    96395      +25     
  Branches    30543    30551       +8     
==========================================
+ Hits        58913    58973      +60     
+ Misses      21411    21358      -53     
- Partials    16046    16064      +18     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

[dsberry]

A few points about your code changes:

• You've made it a static function (like, say, astTune) rather than a virtual function (like, say, astCopy). This means it can't be extended or replaced by any other class of AST object. That's probably okay for the moment, but it's not inconceivable that a subclass may want to replace or extend it at some time in the future. Just something to be aware of.
• The current version of astGetKeymap returns the pointer from the class structure directly, meaning that it's imperative that calling code does not annul the returned pointer. This is a bit dangerous because AST in all other (I think) circumstances returns a cloned pointer and expects the caller to annul it. Having one place that is different is likely to cause confusion. For instance, look at the GetFrame method in frameset.c, which uses astClone to return a cloned pointer. In general, AST doesn't use "borrowed" pointers derived from direct pointer assignment. For consistency, I'd return "astClone(this->keymap)" and change the prologue documentation to say the pointer should be annulled using astAnnul when no longer needed.
• Just a reminder that in AST "cloning" a pointer refers to returning a new reference to an existing object.
• Prior to the addition of "keymap" the Object class structure contained no references to other Objects, unlike say FrameSet that contains many references to other Objects. So we now need to include the keymap in the GetObjSize and ManageLock functions in object.c. You could use the versions of these functions in frameset.c to see how the code should look.

Otherwise, it all looks good.

[embray]

Huh, to my mind returning a borrowed reference seemed to make more sense / be more convenient, so I documented it as such. But if it would be inconsistent with how literally everything else works I'm happy to change that.

Not sure why a subclass would want to extend this--maybe to provide a default, non-empty keymap? Would be easy enough to change later if needed.

I'll look at GetObjSize and ManageLock.

[embray]

Should be better now

• astGetKeyMap returns a new reference via astClone
• GetObjSize and ManageLock accounted for

The more I dig into the internals of how this library works the more impressed I am with how well it's designed; it's really a pleasure to work with. Making each Object hierarchy uniquely owned by a single thread is a good call.

[embray]

Oh, forgot to mention, I also added ast_tester/* to the paths extracted by lcov for the coverage report. This way when running the coverage report locally I can also get coverage on the tests which is very useful.

I was for a while scratching my head about why codecov's report includes all sources, even those that aren't included in the uploaded lcov.info. Turns out codecov's tool actually runs over the entire build directory automatically, extracting any coverage information it can find, including raw gcov outputs, and incorporate that. That can be disabled by passing it disable_search: true in the codecov-action configuration, but I left that out for now.

But by ast_tester code through lcov we can also use things like LCOV_EXCL_LINE on lines that should not be reached if the tests are passing, improving overall line coverage.

[embray]

Updated to include an astHasKeyMap companion method, which can be used to check if an Object has an associated KeyMap without creating an empty one on the fly as astGetKeyMap does (working on integrating this into yamlchan).

I'm still having some doubts as to whether it's even the right call to include this in the astDump serialization, since it's only intended right now for internal use, and maybe shouldn't leak into the serialization format.

[dsberry]

Your call. The default assumption would be that it is dumped, along with every other aspect of the Object. So if you decide not to include it in the dump, then it should be very clearly documented somewhere.

[embray]

Right. I think that's why it's best to keep for now.

[embray]

Is this OK to merge? I've also tried it out a bit in #66 and it will help with some other transforms I'm adding support for.

[embray]

(P.S. @timj I saw you gave me merge permissions anyways, but I want to make sure not to step on any toes)

[dsberry]

Merging is okay with me