[libc-commits] [PATCH] D117258: [libc] add core parsing for printf

[Michael Jones via Phabricator via libc-commits]

michaelrj added a comment.

I think that the ideas you have proposed are good ones, but I think that sticking with the function oriented approach is better than changing to the stream oriented approach.

To start off with, the parsing, formatting, and production functions are all already separate (see `parse_format_specifier_sequential`, `string_conversion`, and the `OutputBuffer` class respectively). These can be tested separately, and I plan on making the future conversions tested separately by default, although I haven't set up separate testing for the string conversions since to test the completed sprintf function we need at least one conversion, and so using the string conversion for testing there works nicely.

In general concept, the stream oriented and function oriented approaches are basically the same, the main difference being that for the existing function oriented approach there isn't an overarching stream class storing the flags and options, they're just passed as arguments to each function. For production the `OutputBuffer` class takes strings and appends them to the end of its buffer, the only difference being that it uses the `write` function instead of the stream operators.

The most important part, in my opinion, is that the function oriented approach is already implemented, and for your other suggestions I can add them to the existing implementation (e.g. changing the way that the buffer is stored in the OutputBuffer). I don't see any particular improvements by adopting the stream approach that make it worth refactoring the implementation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117258/new/

https://reviews.llvm.org/D117258