Deduplicate string literal rendering, fix 4909 #8193
Conversation
github-actions
bot
added
the
with-tests
src/libexpr/value/print.hh
Outdated
| * | ||
| * @param s The logical string | ||
| */ | ||
| std::ostream & printLiteral(std::ostream & o, std::string_view s); |
There was a problem hiding this comment.
I'm not sure if this overloaded printLiteral function (as opposed to printBool, printString etc.) is a good idea. For instance, at some point somebody is going to do printLiteral(null), expecting a null value to be printed, but instead it will call the char * overload...
There was a problem hiding this comment.
Done. It's printLiteralString now, so that it's explicit which kind of literal will be rendered; same for printLiteralBool.
I've kept the "literal" part because I've renamed the file to cover relevant parts of the whole language, not just the values.
| * | ||
| * @param res Where to print to | ||
| * @param s Which logical string to print | ||
| */ |
There was a problem hiding this comment.
Maybe this function should be moved to print.cc?
There was a problem hiding this comment.
Which print.cc? This is for the aterm printing of dervations, and part of the store layer.
There was a problem hiding this comment.
I certainly wouldn't mind say having derivation/{aterm,json}.{cc.hh} to separate data types from serialization, and put both serializations on equal footing, but that seems like a separate PR to me.
There was a problem hiding this comment.
Not feasible, because that's a different library. We should keep these separate. As mentioned in the comment, this is a different function.
I can remove this mention of the language completely if we want to be very (overly?) thorough about libstore not depending on libexpr. Personally I think there's value in cross-referencing components in both directions, although this may not be the brightest example.
src/libexpr/value/print.cc
Outdated
| @@ -0,0 +1,26 @@ | |||
| #include "value/print.hh" | |||
There was a problem hiding this comment.
It seems a bit arbitrary to introduce a "value" subdirectory, when nothing else value-related is there...
There was a problem hiding this comment.
We already have a value subdirectory. I previously made value/context.{hh,cc} on analogy with primops/context.cc. String contexts are part of string values.
There was a problem hiding this comment.
While value/ was not a terrible choice, generalizing the file to allow printing all of the language is more useful. I've renamed it to just print.{cc,hh} in the libexpr top level.
github-actions
bot
added
the
repl
Generalizes the file to sensibly allow printing any part of the language syntax.
|
I've moved the file to the libexpr top level, so that it's more useful.
|
Motivation
Repay some tech debt and fix #4909
Context
Checklist for maintainers
Maintainers: tick if completed or explain if not relevant
tests/**.shsrc/*/teststests/nixos/*Priorities
Add 馃憤 to pull requests you find important.