Wrap deck output at 132 characters - #5335
Open
kriben wants to merge 1 commit into
Open
Conversation
DeckOutput broke a record into lines by counting items only, so keywords whose records hold more than fmt.columns items were split, while data keywords such as SWOF or PVTO were written as one line per record. A rel. perm. table with 40 rows became a single line of more than a thousand characters, and Eclipse 100 rejects lines longer than 132 characters. Decks written by FileDeck could therefore not be run by that simulator. Track the width of the line being written and break the record before it grows past fmt.max_line_width, which defaults to 132 and disables the check when set to zero. A value is measured before it is written, so a break never falls inside a quoted string. Values are formatted through format_value() and written by write_token() to make the width known before anything reaches the stream.
bska
added
the
manual:irrelevant
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
current_width tracking can become incorrect when write_string() outputs newline-containing strings, and format_value(double) does not preserve the output stream locale, both of which can lead to incorrect or inconsistent wrapping/formatting.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates DeckOutput to enforce a maximum output line width (defaulting to 132 characters to satisfy Eclipse 100 constraints) by pre-formatting tokens to measure their width before writing, and wrapping records before they exceed the configured limit.
Changes:
- Add
fmt.max_line_width(default132,0disables) and trackcurrent_widthwhile writing. - Refactor record writing to format tokens first (
format_value) and write viawrite_tokenso wrapping decisions can be made before streaming. - Add regression tests covering bounded width wrapping, unlimited width, and round-trip parsing of a wrapped deck.
File summaries
| File | Description |
|---|---|
| tests/parser/DeckTests.cpp | Adds tests for max line width wrapping, unlimited width mode, and deck round-trip parsing under 132-char constraints. |
| opm/input/eclipse/Deck/DeckOutput.hpp | Extends formatting options with max_line_width and adds internal width tracking/hooks for tokenized writing. |
| opm/input/eclipse/Deck/DeckOutput.cpp | Implements width tracking/wrapping and token pre-formatting before writing to the stream. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
55
to
58
| void DeckOutput::write_string(const std::string& s) { | ||
| this->os << s; | ||
| this->current_width += s.size(); | ||
| } |
Comment on lines
86
to
93
| template <> | ||
| void DeckOutput::write_value( const double& value ) { | ||
| this->os << value; | ||
| std::string DeckOutput::format_value( const double& value ) { | ||
| std::ostringstream ss; | ||
| ss.flags( this->os.flags() ); | ||
| ss.precision( this->os.precision() ); | ||
| ss << value; | ||
| return ss.str(); | ||
| } |
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.
DeckOutput broke a record into lines by counting items only, so keywords whose records hold more than fmt.columns items were split, while data keywords such as SWOF or PVTO were written as one line per record. A rel. perm. table with 40 rows became a single line of more than a thousand characters, and Eclipse 100 rejects lines longer than 132 characters. Decks written by FileDeck could therefore not be run by that simulator.
Track the width of the line being written and break the record before it grows past fmt.max_line_width, which defaults to 132 and disables the check when set to zero. A value is measured before it is written, so a break never falls inside a quoted string. Values are formatted through format_value() and written by write_token() to make the width known before anything reaches the stream.