Unidata / netcdf-java

The Unidata netcdf-java library
https://docs.unidata.ucar.edu/netcdf-java/current/userguide/index.html
BSD 3-Clause "New" or "Revised" License
146 stars 71 forks source link

Fix EnumTypedef name problem - Attempt 2 #1151

Closed DennisHeimbigner closed 1 year ago

DennisHeimbigner commented 1 year ago

Description of Changes

re: Issue https://github.com/Unidata/netcdf-java/issues/126

The code for handling enum types in Group.java is incorrect. When creating a new enum type, it is appropriate to search only the current group for a conflicting name and this is what the current code does. But when an enum typed variable searches for the matching enum type, it must search not only the current group but all parent groups and the current code does not do that.

The fix consists of two similar parts.

  1. Modify Group.findEnumeration to take an extra boolean parameter to control if search is this group only or to search up the group's parents.
  2. Modify Group.Builder.findEnumTypedef to act like part 1 but to search the sequence of parent Group.Builder instances.

As a consequence, this PR modifies a number of other files to adapt to the modified signatures.

Note

This same problem appears to affect the opaque type also, but fixing that may be a bit more difficult because CDM appears to convert opaque types to byte typess.

PR Checklist

Addendum 1

It turns out that I missed the error in the code in H5headerNew that attempts to match an enum typed variable with the proper enumeration type declaration.

The problem and fix (as described by a comment in H5headerNew) is a bit of a hack. It should be fixed in H5Object.read(). Unfortunately, information is not being passed down so that the proper fix can be applied early in the HDF5->CDM translation. Fixing this would affect a lot of function signatures.

Also modified TestEnumTypedef.java to test two cases:

  1. the actual enum type def is in the same group as the variable that uses it.
  2. the actual enum type def is in a parent group of the variable that uses it

Misc. Other Changes

Addendum 2 (12/19/2022)

Addendum 3 (2/25/2023)

Make additional changes to fix Jenkins failures.

tdrwenski commented 1 year ago

Thanks for making those changes Dennis! Is this ready to merge now?

DennisHeimbigner commented 1 year ago

Actually, let me run it with Jenkins one last time.

DennisHeimbigner commented 1 year ago

ok, it still passes Jenkins, so it is ready to merge.