fix: preserve original scalar text for null nodes so as<string> returns the source text#1460
Closed
mvanhorn wants to merge 2 commits into
Closed
fix: preserve original scalar text for null nodes so as<string> returns the source text#1460mvanhorn wants to merge 2 commits into
mvanhorn wants to merge 2 commits into
Conversation
…ns the source text Fixes jbeder#1290
Five-line follow-up per bot review; full suite green.
Author
|
Addressed in 30eade1. Full CTest suite (1029 tests) passes locally. |
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.
Summary
Plumb the original token text through the null path without breaking the public EventHandler API: add a non-pure virtual
OnNull(const Mark&, anchor_t, const std::string& value)overload to include/yaml-cpp/eventhandler.h whose default implementation delegates to the existing two-argOnNull, so third-party handlers and the emitter/graphbuilder adapters keep working unchanged.SingleDocParser::HandleNodepassestoken.valueat the null-plain-scalar site;NodeBuilderoverrides the three-argOnNulland calls a newnode_data::set_null(const std::string& scalar)overload (include/yaml-cpp/node/detail/node_data.h + src/node_data.cpp) that setsm_type = NodeType::Nullwhile storing the source text inm_scalar, exposed via the existingScalar()accessor. Finally, the twoas_if<std::string>specializations in include/yaml-cpp/node/impl.h returnnode.Scalar()for Null nodes instead of the literal"null", sokey:yields"",key: ~yields"~", andkey: nullyields"null", whileIsNull()stays true for all of them.Why this matters
Since yaml-cpp 0.8.0, any null-like plain scalar (
<empty>,~,null,Null,NULL) loses its original text:SingleDocParser::HandleNode(src/singledocparser.cpp, ~line 101) detectsIsNullString(token.value...)and emitsOnNull(mark, anchor), discardingtoken.value.NodeBuilder::OnNullthen callsnode.set_null(), which leavesm_scalarempty, andas_if<std::string>in include/yaml-cpp/node/impl.h (~lines 119 and 151) hard-codes the return value"null"forNodeType::Null. The reporter cannot distinguishmessage:(empty) frommessage: nullbecause bothScalar()andas<std::string>()return fabricated values. Owner jbeder agreed the original text representation should be kept inScalar()and returned byas<std::string>()("Yeah that's reasonable").See #1290.
Testing
message: nulland assertIsNull()is true,Scalar() == "null", andas<std::string>() == "null"(test/integration/load_node_test.cpp). - Variants:key:yields"",key: ~yields"~",key: Null/key: NULLyield their exact source text; quoted"null"remains a plain string scalar unaffected. - Edge cases: flow-map and block-seq missing values (structural nulls with no token) return""fromas<std::string>()and stayIsNull(); a default-constructedNode()still reports Null with empty scalar (test/node/node_test.cpp). - Error paths:as<std::string>()on a map/sequence still throwsTypedBadConversion;as<std::string>(fallback)on non-scalar non-null still returns the fallback.Fixes #1290