Align the documentation with the API the library actually has - #27
Merged
Conversation
The examples across the documentation site named an API this library does not declare: is_number()/as_number() rather than the is<T>()/as<T>() templates, dictionary::set and get rather than operator[], array::size rather than length, environment::set/unset/get_all next to a value() that is the only thing there, path::join/exists/read_file where the class has mkdir, home and separator, an error_code type, and parse_json/to_json for a library whose format support is TOML. The include paths were no better: of the 32 dross includes in the docs, 31 named headers that have never existed -- dross/value.h for what lives at dross/type/value.h, and so on. Nothing on the site compiled except the one snippet in the README. The examples now use what the headers declare, and every code block that includes a dross header was extracted and compiled to confirm it: 22 blocks, twice each, once against libstdc++ 15 with Clang 22 and once against libstdc++ 13 with Clang 20. The harness adds no headers of its own -- a block that needs <iostream> says so -- because a block that only compiles inside a harness is not an example a reader can use. Where the library has no equivalent for what a passage showed, the passage now shows what the library does have: TOML in place of JSON, mkdir in place of create_directory, the four xdg accessors reached through an instance. Where nothing equivalent exists, the lines are gone rather than replaced with a placeholder. No section had to be dropped whole. The Code of Conduct paragraph goes too, since the file it points at is not in the tree.
The rewrite took its wording from the Doxygen comments on the headers, and four of those describe behaviour the implementation does not have. So the examples inherited the errors: length() counts UTF-8 bytes rather than code points (src/type/string.cpp:41), as<T>() on a mismatch returns a default-constructed T rather than being undefined (src/type/value.cpp:169), mkdir() reports failure when the directory is already there rather than succeeding (src/platform/path.cpp:19), and value() yields an optional holding an empty string for a variable set to nothing, rather than nullopt (src/platform/environment.cpp:9). Both the prose and those four comments now say what the code does. The comments are the only thing changed under include/; no code moved. For as<T>() the wording stops at "call is<T>() first, the result is unspecified otherwise, and this does not throw". Saying a default T comes back would document an accident: the contract calls the case undefined, so nothing should be built on top of what it happens to return today. Two implementation faults get written down rather than smoothed over. mkdir()'s failure on an existing directory carries no diagnostic code, so the error reads "failed: Success". And expand() lets a filesystem_error escape when the target is missing, though its return type says otherwise -- resolve() catches the same condition. The pages say so, and point at resolve() for paths that may not exist yet. The JSON example page goes: the library parses TOML, and no json entry point exists. Its toctree entry and cross-reference go with it, leaving no dangling links.
The previous commit fixed four places and left their neighbours, which was worse than leaving all of them: before, the headers and the prose agreed with each other and were uniformly wrong; after, mkdir appeared twice on one page saying opposite things, and the corrected length() rendered directly below an example still labelled "Unicode-aware length". Three lenses and the independent reviewer each found the same three sites. So the sweep now goes by wording rather than by line number. mkdir's filesystem::path overload -- the one that actually holds the failure check, the string overload merely forwards to it -- says what the string one says. The class example no longer claims a Unicode-aware length, and value_cast no longer advertises a std::bad_cast it is declared noexcept against. The guidance to call exists() before mkdir is gone. exists() reaches for the throwing std::filesystem::exists, so it was not the safe guard it was offered as. The page states the behaviour and leaves it there. Two more throwing paths join the list of exceptions to the "reports failures through the return type" rule: exists() itself and the default path constructor, which calls std::filesystem::absolute. And expand() escapes on any canonicalisation failure, not only on a missing target. Finally, the string class stops describing itself as Unicode-aware. It holds UTF-8 bytes and works on them: length() counts bytes, prefix and equality compare bytes, and either can split or match across a multi-byte character. Nothing validates the content, so the documentation no longer promises undefined behaviour for malformed input -- the bytes are kept and handed back as they came.
Three of the corrections in the last commit were themselves wrong, and each was introduced by the correction rather than inherited from the base. as<T>() was written up as leaving an unspecified result on a mismatch. The reasoning was that the header called the case undefined, so nothing should be built on what the implementation happens to return -- except that test/type/value_cast_wrong_type pins the return to a default-constructed T with EXPECT_EQ. Change the implementation and the suite fails, which makes it a contract whatever the comment said. The documentation now says a default-constructed T comes back, and still says to call is<T>() first. The string example claimed a length of 14 bytes on a line that runs after an append, and reached that append through an operator+ the class does not have. And mkdir's failure on an existing directory was described as indistinguishable from a real one, which overstates it: create_directories fills in the error code, so a caller reading error().code() can usually tell the two apart. Withdrawing the exists() guard stands -- exists() throws and races -- but the reason given for it was wrong. The rest is the same class of staleness, found further out each time the net widened. README still called strings Unicode-aware. platform.h, dross.h, index.rst and the user guide still promised no exceptions at all. number.h advertised a std::runtime_error on three conversions that return 0, 0.0 and 0LL instead, and described rounding as truncation. The changelog credited every type with three-way comparison and with value semantics, when six types have the former and environment, being a static utility, has neither. A caveat list said two and held three. Which is the end of it for this branch. What the sweep did not reach is written down rather than left to be discovered again.
The previous commit replaced "for all types" with "for every class except environment", which reads wider than the twelve types the claim was checked against. Four nested iterator classes hold a unique_ptr and never declare copy assignment, so the implicit one is deleted and "every class" is false of them. The earlier wording was not, since the changelog lists types rather than classes. Both places that carry the claim now stay inside the list they follow, and they agree with each other again -- the user guide had kept saying all types were copyable while the changelog no longer did.
Two more places said all types carry value semantics. The API index is plainly wrong there -- its own toctree renders dross::environment, whose special members are all deleted -- and it sits in the same file that carefully lists four exceptions to the no-exceptions rule twenty lines above. The README is merely ambiguous: read against its Type System list the sentence holds, read against the library it does not. Both now name the value types and put environment outside, as the changelog and the user guide already do.
The quick example in the README used three things the library does not have: a braced initialiser list for dictionary, a static xdg::config_home, and operator/ on path. It was never part of the block sweep -- that covered the pages under docs/sphinx/source -- so nothing had ever compiled it. It now builds and runs, and says in passing why the entries are assigned after construction and why xdg needs an instance. Three more places promised errors in the return type without mentioning that path reaches for throwing filesystem calls in three places, on top of the bounds-checked accessors they already named. The contributing guide keeps its "no exceptions" line: it sits among instructions to contributors, next to "use Pimpl idiom" and "apply const, constexpr and noexcept", and is copied from the style guide. It says how to write new code rather than what the library does today. That the library does not follow it is a separate matter, and is recorded as one.
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.
What
The documentation described an API this library does not have. Every code
sample that included a dross header named a path that has never existed, and
most of them called functions that are not declared anywhere. Nothing on the
documentation site compiled except the one snippet in the README.
This rewrites the samples against what the headers declare, and corrects the
prose — including the Doxygen comments the prose was copied from, which turned
out to be the source of several of the errors.
What the samples were saying
#include <dross/value.h>and 30 othersinclude/dross/type/value.h; 31 of 32 include paths named files that are not therevalue::is_number()/as_number()is<T>()/as<T>()templatesdictionary::set(k, v)/get(k)operator[],contains()array::size()length()environment::get/set/unset/get_allstatic std::optional<std::string> value()path::join/absolute/exists/read_file/write_filemkdir,home,separator,append,expand,resolve,existserror_codetype,parse_json/to_jsonWhere the library has an equivalent, the sample now uses it — TOML in place of
JSON,
mkdirin place ofcreate_directory, the fourxdgaccessors reachedthrough an instance. Where it has none, the lines are gone rather than replaced
with a placeholder. One page went with them: the JSON example, since there is no
JSON entry point to document. Its toctree entry and cross-reference went too.
Verification
Every code block that includes a dross header was extracted and compiled: 22
blocks, twice each, once with Clang 22 against libstdc++ 15 and once with
Clang 20 against libstdc++ 13. All 22 pass in both configurations with
-Wall -Wextraand no warnings from the samples themselves.The harness adds no headers of its own — a block that needs
<iostream>says so— because a block that only compiles inside a harness is not an example a reader
can use. Five blocks were linked and run to check that their output matches what
the pages claim.
Compiling is not the same as being right, so the claims were checked against the
implementation rather than against the headers. That distinction mattered: the
first pass took its wording from the Doxygen comments, and four of those describe
behaviour the code does not have.
length()counts Unicode code pointsbuffer.size()— UTF-8 bytes."日本語"is 9as<T>()on a mismatch is undefinedT, andvalue_cast_wrong_typepins that withEXPECT_EQmkdir()succeeds if the directory existserror().code()is zero there and non-zero for real failuresvalue()yieldsnulloptfor an empty variablegetenvreturns non-null forFOO=, so the optional holds""numberconversions throwstd::runtime_error0,0.0and0LLThose comments are now corrected too. Only comments changed under
include/; nocode moved.
Two implementation faults are written down rather than smoothed over
path::expand()declaresstd::expectedbut lets afilesystem_errorescapewhen canonicalisation fails — a missing target, a permission problem, a symlink
loop.
resolve()catches the same condition. Measured: the process terminateswith SIGABRT.
mkdir()treats an existing directory as a failure, and the error it returnscarries no diagnostic code, so
what()reads "failed: Success".The pages say so, and point at
resolve()for paths that may not exist yet.Neither is worked around in the prose as though it were intended behaviour.
Claims that were wider than the code
stringis no longer described as Unicode-aware: it holds UTF-8 bytes, andlength(), prefix and equality all work on those bytes, so any of them can splitor match across a multi-byte character. Nothing validates the content either, so
the promise of undefined behaviour on malformed input is gone — the bytes are
kept and handed back as they came.
Four pages said all types are copyable and assignable;
environmenthas all itsspecial members deleted. Four pages said the library does not use exceptions; the
bounds-checked accessors throw
std::out_of_range, and threepathoperationsreach for throwing filesystem calls. The changelog credited every type with
three-way comparison, where six have it.
The README example
The sweep covered the pages under
docs/sphinx/source, so the quick example inthe README was outside it and had never been compiled. It used a braced
initialiser list for
dictionary, a staticxdg::config_home(), andoperator/on
path— none of which exist. It now builds and runs in both configurations,and mentions in passing why the entries are assigned after construction and why
xdgtakes an instance.What this does not fix
The library has faults that documentation cannot close, and pages that were left
alone. Both are listed rather than left to be found again:
path::expand()terminating, andmkdir()returning an error with no codevalue{x}wrapping its argument in a one-elementarray, so a type changessilently
path,xdganderrorhaving no tests, which is why several of these wentunnoticed
@codeexamples in the headers that do not compile, including ones that usethe
operator/the README no longer usestoml.hclaiming complete TOML v1.0.0 support, which the serializer does notprovide
The contributing guide keeps its "no exceptions" rule: it tells contributors how
to write new code, not what the library does. That the library does not follow
it is one of the faults above.