Put the data dictionary behind an interface, and delete the 10,380 lines nobody read. - #81
Merged
Merged
Conversation
…nes nobody read.
The dictionary was three exported maps: DicomDictionaryGo, RepeatersDictionaryGo
and PrivateDictionaries. Read-only by convention, mutable by type, with no
synchronisation of any kind -- any caller could have corrupted the dictionary for
the whole process, and there was no way to substitute a different one.
Standard() now returns a Dictionary, whose single Lookup(tag, creator) answers
for both halves of the dictionary: creator names the Private Creator of a private
tag and is ignored for a standard one. It resolves an exact entry ahead of a
repeating-group pattern, which is not arbitrary -- (7FE0,0010) is Pixel Data
exactly and retired Variable Pixel Data through the 7Fxx,0010 mask, and every
image ever written means the former -- and it never resolves a private tag
against the standard tables, because the same tag means different things to
different vendors.
The package's own four resolvers go through it, so it is the path godicom takes
rather than a wrapper around one. That costs nothing: go build -gcflags=-m
reports Standard inlined, .Lookup devirtualized and standardDictionary{}
non-escaping at all four call sites. dictionaryHasTag deliberately stays a direct
probe of the exact table -- the endianness heuristic needs "this exact tag is a
known element", and the 88 masks cover 108,688 tags against 5,189 exact entries.
It is also the first public way to read a standard entry's VM, name or retired
flag. LookupVR gave the VR and laundered a missing entry into UN; the
PrivateDictionary* functions covered only private tags.
DictEntry.VRs() splits the compound forms PS3.6 writes as prose. Thirty-seven
entries permit two -- OB or OW is PixelData -- and (0028,1200) Gray Lookup Table
Data permits three, US or SS or OW. vrDisagreesWithDictionary compared the whole
string against one VR before, so it disagreed with every correctly encoded
PixelData element in the world.
Removed:
- dictionaryIsRetired, which consulted only the exact table and so answered
"not retired" for all 72 retired repeating-group entries. No caller outside
its own test.
- the generated tagToKeyword and tagToName maps, 10,380 lines and two
5,000-entry maps built at init, both a second copy of data dicomDictionary
already held one map access away. tagToName had no reader at all; staticcheck
skips generated files, which is why -checks=all never minded.
Two generator bugs fixed on the way. generate_dict.py wrote to
dicom_dict_generated.go, a name nothing in the tree has, so running it produced a
second file redeclaring DictEntry and every map -- the package stopped compiling
instead of the dictionary updating. And neither generator gofmt'd its output, so
regenerating buried the data change under a 5,182-line whitespace diff. Both now
run gofmt, and re-running both is a no-op against the checked-in files.
Breaking at 0.x: the three maps and PrivateDictEntry are unexported. Nothing in
the godicom-dev organisation referenced them. DictEntry stays exported -- it is
what Lookup returns.
Groundwork for #71.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 25, 2026
xbmlz
added a commit
that referenced
this pull request
Aug 25, 2026
…the read. (#82) #81 put the dictionary behind an interface. Nothing could supply one. A private element's VR depends on the vendor who wrote the file, and the only way to teach godicom a vendor's block was AddPrivateDictEntry, which mutates a process-global table: every caller in the process sees the entry, and the only way to remove it clears whatever anyone else registered too. Right for a program that owns its process, wrong for a library. ReadOptions.Dictionary is the per-read version, nil meaning PS3.6. It changes what an element means rather than how it is described -- an implicit VR file carries no VRs at all, so every element's VR is whatever the dictionary says, and for a private element that answer depends on who wrote it. NewPrivateDictionary builds a set of vendor entries that touches no global state, and NewDictionary composes: each member is tried in turn and the first entry found wins, so the argument order is the precedence. Compose rather than replace -- NewDictionary(vendor, Standard()) keeps the other 5,189 entries, and handing over the vendor's dictionary on its own would leave every standard element as UN. Three places the plumbing had to reach, none of them obvious from the signature that needed changing: The dictionary lives on readContext, not codecContext where the rest of the per-element decode state lives, because it has to outlive the parse. A deferred value is decoded on Get, long after the read returned, and deferred.go compares the VR it re-resolves against the one the element was first read under. A dictionary scoped to the parse would turn every deferred implicit-VR private element into "deferred read VR UN does not match original US". readContext is the only piece of parse state a Dataset retains, which is what makes it the right home. Replacing rc.dictionary() with Standard() in datasetResolver reproduces that error in both subtests of TestReadOptionsDictionarySurvivesADeferredLoad. finishDeflated copies ReadOptions field by field, so a Deflated file would have read its inflated dataset against PS3.6 while the same file undeflated resolved properly. Only readReaderAt takes that path -- readBytes inflates in place and keeps its readCtx -- so the test drives ReadFile as well as ReadBytes, and dropping the copy fails it. The VR-mismatch diagnostic is now judged against the dictionary the read was given rather than always PS3.6. A caller who overrode an entry said what they expect the file to contain; measuring against a different expectation than the parse used would be reporting on nothing. Private tags stay exempt even when the supplied dictionary has an entry, because the creator is not in hand at that point and resolving one per element would charge the quiet path for a diagnostic nobody asked for. dictionaryVRForRead and lookupVRWithCreator are replaced by vrResolver, which pairs the dictionary with the private-creator lookup because neither answers alone -- a creator is only worth resolving against a dictionary that has that vendor's block in it. Its zero value resolves against Standard with no creator, which is what a header decoded outside a parse should get. LookupVR is now one call to it. AddPrivateDictEntry and ResetExtraPrivateDictionaries behave exactly as before and are not deprecated; they are now implemented on top of PrivateDictionary rather than a parallel table, and document the global mutation they perform. Three gaps left deliberately, each recorded where someone would look for it: - No WriteOptions.Dictionary. The write path resolves an ambiguous VR from the dataset's own values -- Pixel Representation decides between US and SS -- and never consults the dictionary, so the field would have nothing to do. - DecodeDataset and friends take no options at all and so still resolve against Standard(). Giving four functions an options parameter is a wider change than the dictionary alone justifies. - dictionaryHasTag stays a direct probe of the exact standard table. It runs before the transfer syntax is known, to guess a byte order from the first tag; a private block cannot help, and letting a supplied dictionary answer would make the guess depend on how many entries the caller happened to add. 19 new tests in dictionary_composition_test.go, and ExampleNewPrivateDictionary compile-checks the README snippet. Closes #71. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
xbmlz
added a commit
that referenced
this pull request
Aug 26, 2026
…ce. (#84) The uid package exported three maps: Dictionary (496 registered UIDs and their PS3.6 metadata), KeywordToUID (494 keyword -> UID), and Known (an Info per registered UID, built by a package init). All three were read-only by convention and mutable by type, with no synchronisation of any kind. `uid.Known[uid.ImplicitVRLittleEndian] = someOtherInfo` was one assignment away from making every implicit-VR file in the process decode as explicit, from anywhere in any dependency, and nothing in the package could have noticed. This is the same defect the data dictionary had before #81, in the table that decides how bytes are read. Read access is now two accessors. Lookup takes a UID and returns Info, which is what the three internal call sites already wanted -- DecodeDatasetContext, determineWriteEncoding, EncodeDataset all read IsTransferSyntax, IsImplicitVR and IsLittleEndian off it -- and it returns a copy, so the table cannot be reached through the result. LookupKeyword takes a keyword and returns a UID, which is the direction the old Lookup went, under a name that says which direction that is. Dictionary, KeywordToUID and DictEntry become unexported, in the generator as well as the generated file, so a regeneration cannot quietly export them again. Known is deleted rather than unexported. It was a second copy of the dictionary: every field in an Info is either read straight from the dictionary entry or derived from the UID in three comparisons, so Lookup derives them on demand instead. That leaves one table where there were two, nothing to keep in step, and no init work for a program that never looks a UID up. The root aliases UIDDictionary and KnownUIDs are deleted too -- an alias to another package's identifier cannot be unexported, and there is nothing left for them to name. UIDInfo and LookupUID stay. Verified by mutation, each restored and re-checked: forcing IsImplicitVR false fails TestLookup and eight root tests; answering true for an unregistered UID fails TestLookup and TestDetermineEncodingPrivateTransferSyntaxRequiresArgs; dropping Keyword fails TestLookup, TestLookupCoversEveryEntry and TestLookupKeyword. The first of those found a real gap -- nothing had pinned the implicit-VR true case -- which TestLookup now covers. Full local gate green: build, vet, gofmt, staticcheck -checks=all, tests on amd64 and GOARCH=386, all eight cross-vet targets. Lookup, LookupKeyword and entry are at 100% coverage, and ExtraInfo/IsRetired go from 66.7% to 100%. Within the organisation the only affected code is two lines in one gonetdicom test, ae/storage_sop_classes_test.go:35 and :121, recorded in the CHANGELOG for whoever bumps the module there. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The dictionary was three exported maps:
DicomDictionaryGo,RepeatersDictionaryGoandPrivateDictionaries. Read-only by convention,mutable by type, with no synchronisation of any kind — any caller could have
corrupted the dictionary for the whole process, and there was no way to
substitute a different one.
The interface
Standard()now returns aDictionary, whose singleLookup(tag Tag, creator string) (DictEntry, bool)answers for both halves ofthe dictionary:
creatornames the Private Creator of a private tag and isignored for a standard one.
It resolves an exact entry ahead of a repeating-group pattern, which is not
arbitrary —
(7FE0,0010)is Pixel Data exactly and retired Variable PixelData through the
7Fxx,0010mask, and every image ever written means theformer. It never resolves a private tag against the standard tables, because
the same tag means different things to different vendors.
It is also the first public way to read a standard entry's VM, name or retired
flag.
LookupVRgave the VR and laundered a missing entry intoUN, and thePrivateDictionary*functions covered only private tags. So this is acapability gain, not only groundwork.
The package's own four resolvers go through it, so it is the path godicom itself
takes rather than a wrapper around one. That costs nothing:
go build -gcflags='-m'reportsinlining call to Standard,devirtualizing .Lookup to standardDictionaryandstandardDictionary{} does not escapeat all four call sites.dictionaryHasTagdeliberately stays a direct probe of the exact table. Theendianness heuristic needs "this exact tag is a known element", not "something
in the dictionary covers this tag" — the 88 masks cover 108,688 tags between
them against 5,189 exact entries. No read test in the suite distinguishes the
two, so
TestDictionaryHasTagIgnoresRepeatersis what holds that line.Compound VRs
DictEntry.VRs()splits the forms PS3.6 writes as prose. Thirty-seven entriespermit two —
OB or OWis PixelData — and(0028,1200)Gray Lookup Table Datapermits three,
US or SS or OW.vrDisagreesWithDictionarycompared the whole string against one VR, so itdisagreed with every correctly encoded PixelData element in the world. It uses
VRs()now.Removed
dictionaryIsRetired, which consulted only the exact table and so answered"not retired" for all 72 retired repeating-group entries. It had no caller
outside its own test, so nothing ever noticed.
Retiredis read off the entrynow.
tagToKeywordandtagToNamemaps, 10,380 lines and two5,000-entry maps built at init, both a second copy of data
dicomDictionaryalready held one map access away.
tagToNamehad no reader at all —staticcheck skips generated files, which is why
-checks=allnever minded.keywordToTagstays, because that direction is the one that needs an index.Two generator bugs fixed on the way
generate_dict.pywrote todicom_dict_generated.go, a name nothing in thetree has: the checked-in file is
dictionary_generated.go. Running thegenerator therefore produced a second file redeclaring
DictEntryand everymap in it, so the package stopped compiling instead of the dictionary being
updated.
formatted, so regenerating buried the data change under a 5,182-line
whitespace diff. Both now run
gofmt -w, and re-running both against thecurrent pydicom submodule is a no-op — 5,189 standard entries, 88 repeaters,
449 private creators, 10,545 private entries, byte-identical.
Breaking
At 0.x: the three maps and
PrivateDictEntryare unexported. Nothing in thegodicom-dev organisation referenced them (checked gonetdicom, golibjpeg,
goopenjpeg and the docs site).
DictEntrystays exported — it is whatLookupreturns.
AddPrivateDictEntryremains the way to register a private entry atruntime, and
Lookupfinds those too.Verification
go build ./...,go vet ./...,go test -count=1 -p 4 ./...— greenstaticcheck -checks=all(CI's exact invocation) — cleanGOARCH=386 CGO_ENABLED=0 go test ./...— greenGOARM=5— cleanperturbation falsified a comment I had written about
dictionaryHasTag; thecomment now states the measured magnitudes instead of the claim I could not
support.
ExampleStandard,ExampleDictEntry_VRs,ExampleAddPrivateDictEntry) keep the new README section compile-checked, perf17c828.
Groundwork for #71. Deliberately not in this PR:
ReadOptions.Dictionary/WriteOptions.Dictionarythreading anddictionary.New(...)composition, whichneed their own API round.
🤖 Generated with Claude Code