C++ string Formatter Again Part-2

The name of the pictureThe name of the pictureThe name of the pictureClash Royale CLAN TAG#URR8PPP





.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0;







up vote
4
down vote

favorite
1












Previously asked here.



The code is now available on GitHub.



Since the previous review I have added unit tests.



Since it is big it will come in a couple of parts.



Part 1 | Part 2 | Part 3 | Part 4



Part 2



This is a simple helper object. It stores the result of parsing the string in an object. This way we don't need to re-parse the string for information.



FormatInfo.h



#ifndef THORSANVIL_IOUTIL_FORMAT_INFO_H
#define THORSANVIL_IOUTIL_FORMAT_INFO_H

#include <iostream>
#include <typeinfo>

namespace ThorsAnvil::IOUtil


// Enum representing the Length, specifier and type information provided by the format string
#pragma vera-pushoff
// printf doc http://www.cplusplus.com/reference/cstdio/printf/
enum class Length none, hh, h, l, ll, j, z, t, L; // Use the same values as in printf documentation so it is easy to lookup
enum class Specifier d, i, u, o, x, X, f, F, e, E, g, G, a, A, c, s, p, n; // Use the same values as in printf documentation so it is easy to lookup
enum class Type Int, UInt, Float, Char, String, Pointer, Count; // The type we are expecting
enum class Dynamic None, Width, Precision, Both; // If width and/or precision are specified by parameter
#pragma vera-pop

using AllowedType = std::pair<std::type_info const*, int>;

struct FormatInfo

// Formatter has the text representation:
// %<Flag>*<Width>?[.<Precision>]?<Length>?<Specifier>
//
// Flag: - + <space> # 0
// Width: 1-90-9*
// Precision: 0-9*
// Length: hh h l ll j z t L
// Specifier: d i u o x X f F e E g G a A c s p n

// Flags
bool leftJustify; // - Left-justify within the given field width; Right justification is the default (see width sub-specifier).
bool forceSign; // + Forces to precede the result with a plus or minus sign (+ or -) even for positive numbers. By default, only negative numbers are preceded with a - sign.
bool forceSignWidth; // (space) If no sign is going to be written, a blank space is inserted before the value.
bool prefixType; // # Used with o, x or X specifiers the value is preceded with 0, 0x or 0X respectively for values different than zero.
// Used with a, A, e, E, f, F, g or G it forces the written output to contain a decimal point even if no more digits follow. By default, if no digits follow, no decimal point is written.
bool leftPad; // 0 Left-pads the number with zeroes (0) instead of spaces when padding is specified (see width sub-specifier).
// Width and precision of the formatter.
std::size_t width;
std::size_t precision;
Length length;
Specifier specifier;

// Pre-calculate a some values based on the format string
Type type; // Calculated based on length/specifier
Dynamic useDynamicSize; // Are width/precision dynamically specified.
AllowedType expectedType; // Type info we expect the next argument to be based on length/specifier
std::ios_base::fmtflags format; // The format flags that will be applied to stream before the next argument

;



#endif






share|improve this question



























    up vote
    4
    down vote

    favorite
    1












    Previously asked here.



    The code is now available on GitHub.



    Since the previous review I have added unit tests.



    Since it is big it will come in a couple of parts.



    Part 1 | Part 2 | Part 3 | Part 4



    Part 2



    This is a simple helper object. It stores the result of parsing the string in an object. This way we don't need to re-parse the string for information.



    FormatInfo.h



    #ifndef THORSANVIL_IOUTIL_FORMAT_INFO_H
    #define THORSANVIL_IOUTIL_FORMAT_INFO_H

    #include <iostream>
    #include <typeinfo>

    namespace ThorsAnvil::IOUtil


    // Enum representing the Length, specifier and type information provided by the format string
    #pragma vera-pushoff
    // printf doc http://www.cplusplus.com/reference/cstdio/printf/
    enum class Length none, hh, h, l, ll, j, z, t, L; // Use the same values as in printf documentation so it is easy to lookup
    enum class Specifier d, i, u, o, x, X, f, F, e, E, g, G, a, A, c, s, p, n; // Use the same values as in printf documentation so it is easy to lookup
    enum class Type Int, UInt, Float, Char, String, Pointer, Count; // The type we are expecting
    enum class Dynamic None, Width, Precision, Both; // If width and/or precision are specified by parameter
    #pragma vera-pop

    using AllowedType = std::pair<std::type_info const*, int>;

    struct FormatInfo

    // Formatter has the text representation:
    // %<Flag>*<Width>?[.<Precision>]?<Length>?<Specifier>
    //
    // Flag: - + <space> # 0
    // Width: 1-90-9*
    // Precision: 0-9*
    // Length: hh h l ll j z t L
    // Specifier: d i u o x X f F e E g G a A c s p n

    // Flags
    bool leftJustify; // - Left-justify within the given field width; Right justification is the default (see width sub-specifier).
    bool forceSign; // + Forces to precede the result with a plus or minus sign (+ or -) even for positive numbers. By default, only negative numbers are preceded with a - sign.
    bool forceSignWidth; // (space) If no sign is going to be written, a blank space is inserted before the value.
    bool prefixType; // # Used with o, x or X specifiers the value is preceded with 0, 0x or 0X respectively for values different than zero.
    // Used with a, A, e, E, f, F, g or G it forces the written output to contain a decimal point even if no more digits follow. By default, if no digits follow, no decimal point is written.
    bool leftPad; // 0 Left-pads the number with zeroes (0) instead of spaces when padding is specified (see width sub-specifier).
    // Width and precision of the formatter.
    std::size_t width;
    std::size_t precision;
    Length length;
    Specifier specifier;

    // Pre-calculate a some values based on the format string
    Type type; // Calculated based on length/specifier
    Dynamic useDynamicSize; // Are width/precision dynamically specified.
    AllowedType expectedType; // Type info we expect the next argument to be based on length/specifier
    std::ios_base::fmtflags format; // The format flags that will be applied to stream before the next argument

    ;



    #endif






    share|improve this question























      up vote
      4
      down vote

      favorite
      1









      up vote
      4
      down vote

      favorite
      1






      1





      Previously asked here.



      The code is now available on GitHub.



      Since the previous review I have added unit tests.



      Since it is big it will come in a couple of parts.



      Part 1 | Part 2 | Part 3 | Part 4



      Part 2



      This is a simple helper object. It stores the result of parsing the string in an object. This way we don't need to re-parse the string for information.



      FormatInfo.h



      #ifndef THORSANVIL_IOUTIL_FORMAT_INFO_H
      #define THORSANVIL_IOUTIL_FORMAT_INFO_H

      #include <iostream>
      #include <typeinfo>

      namespace ThorsAnvil::IOUtil


      // Enum representing the Length, specifier and type information provided by the format string
      #pragma vera-pushoff
      // printf doc http://www.cplusplus.com/reference/cstdio/printf/
      enum class Length none, hh, h, l, ll, j, z, t, L; // Use the same values as in printf documentation so it is easy to lookup
      enum class Specifier d, i, u, o, x, X, f, F, e, E, g, G, a, A, c, s, p, n; // Use the same values as in printf documentation so it is easy to lookup
      enum class Type Int, UInt, Float, Char, String, Pointer, Count; // The type we are expecting
      enum class Dynamic None, Width, Precision, Both; // If width and/or precision are specified by parameter
      #pragma vera-pop

      using AllowedType = std::pair<std::type_info const*, int>;

      struct FormatInfo

      // Formatter has the text representation:
      // %<Flag>*<Width>?[.<Precision>]?<Length>?<Specifier>
      //
      // Flag: - + <space> # 0
      // Width: 1-90-9*
      // Precision: 0-9*
      // Length: hh h l ll j z t L
      // Specifier: d i u o x X f F e E g G a A c s p n

      // Flags
      bool leftJustify; // - Left-justify within the given field width; Right justification is the default (see width sub-specifier).
      bool forceSign; // + Forces to precede the result with a plus or minus sign (+ or -) even for positive numbers. By default, only negative numbers are preceded with a - sign.
      bool forceSignWidth; // (space) If no sign is going to be written, a blank space is inserted before the value.
      bool prefixType; // # Used with o, x or X specifiers the value is preceded with 0, 0x or 0X respectively for values different than zero.
      // Used with a, A, e, E, f, F, g or G it forces the written output to contain a decimal point even if no more digits follow. By default, if no digits follow, no decimal point is written.
      bool leftPad; // 0 Left-pads the number with zeroes (0) instead of spaces when padding is specified (see width sub-specifier).
      // Width and precision of the formatter.
      std::size_t width;
      std::size_t precision;
      Length length;
      Specifier specifier;

      // Pre-calculate a some values based on the format string
      Type type; // Calculated based on length/specifier
      Dynamic useDynamicSize; // Are width/precision dynamically specified.
      AllowedType expectedType; // Type info we expect the next argument to be based on length/specifier
      std::ios_base::fmtflags format; // The format flags that will be applied to stream before the next argument

      ;



      #endif






      share|improve this question













      Previously asked here.



      The code is now available on GitHub.



      Since the previous review I have added unit tests.



      Since it is big it will come in a couple of parts.



      Part 1 | Part 2 | Part 3 | Part 4



      Part 2



      This is a simple helper object. It stores the result of parsing the string in an object. This way we don't need to re-parse the string for information.



      FormatInfo.h



      #ifndef THORSANVIL_IOUTIL_FORMAT_INFO_H
      #define THORSANVIL_IOUTIL_FORMAT_INFO_H

      #include <iostream>
      #include <typeinfo>

      namespace ThorsAnvil::IOUtil


      // Enum representing the Length, specifier and type information provided by the format string
      #pragma vera-pushoff
      // printf doc http://www.cplusplus.com/reference/cstdio/printf/
      enum class Length none, hh, h, l, ll, j, z, t, L; // Use the same values as in printf documentation so it is easy to lookup
      enum class Specifier d, i, u, o, x, X, f, F, e, E, g, G, a, A, c, s, p, n; // Use the same values as in printf documentation so it is easy to lookup
      enum class Type Int, UInt, Float, Char, String, Pointer, Count; // The type we are expecting
      enum class Dynamic None, Width, Precision, Both; // If width and/or precision are specified by parameter
      #pragma vera-pop

      using AllowedType = std::pair<std::type_info const*, int>;

      struct FormatInfo

      // Formatter has the text representation:
      // %<Flag>*<Width>?[.<Precision>]?<Length>?<Specifier>
      //
      // Flag: - + <space> # 0
      // Width: 1-90-9*
      // Precision: 0-9*
      // Length: hh h l ll j z t L
      // Specifier: d i u o x X f F e E g G a A c s p n

      // Flags
      bool leftJustify; // - Left-justify within the given field width; Right justification is the default (see width sub-specifier).
      bool forceSign; // + Forces to precede the result with a plus or minus sign (+ or -) even for positive numbers. By default, only negative numbers are preceded with a - sign.
      bool forceSignWidth; // (space) If no sign is going to be written, a blank space is inserted before the value.
      bool prefixType; // # Used with o, x or X specifiers the value is preceded with 0, 0x or 0X respectively for values different than zero.
      // Used with a, A, e, E, f, F, g or G it forces the written output to contain a decimal point even if no more digits follow. By default, if no digits follow, no decimal point is written.
      bool leftPad; // 0 Left-pads the number with zeroes (0) instead of spaces when padding is specified (see width sub-specifier).
      // Width and precision of the formatter.
      std::size_t width;
      std::size_t precision;
      Length length;
      Specifier specifier;

      // Pre-calculate a some values based on the format string
      Type type; // Calculated based on length/specifier
      Dynamic useDynamicSize; // Are width/precision dynamically specified.
      AllowedType expectedType; // Type info we expect the next argument to be based on length/specifier
      std::ios_base::fmtflags format; // The format flags that will be applied to stream before the next argument

      ;



      #endif








      share|improve this question












      share|improve this question




      share|improve this question








      edited Mar 4 at 19:51









      Phrancis

      14.6k645137




      14.6k645137









      asked Mar 4 at 19:19









      Martin York

      70.9k481244




      70.9k481244




















          1 Answer
          1






          active

          oldest

          votes

















          up vote
          3
          down vote













          Very little code prompts a very small review. Here are a few things that came to my mind:



          Includes



          One issue is definitely that you are missing #include <utility> for std::pair, as well as #include <cstddef> (or similar) for std::size_t.



          Another thing that might turn out to make some people angry at you is the fact that you #include <iostream>. The reason for this is that this header injects some static initializers into the current compilation unit which initialize the special streams it provides, which in turn leads to binary bloat and a possible performance reduction (take a look at the assembly that your header generates).



          On almost all occasions, #include <iostream> can be avoided by including one of several other headers, in you case #include <ios>, which provides std::ios_base.



          Ugh, std::type_info



          Usually, seeing std::type_info around is setting off some alarm bells for me. The cause for this is not that there is anything inherently wrong with std::type_info, but that most things it could be used for are better expressed through polymorphism or compile time template switching.



          From the single file you provide, I cannot possibly tell whether that is the case with your code or not. However, passing a pointer to some std::type_info around together with an int does hint at somewhat dubious practices. Please reevaluate whether the use of std::type_info is really appropriate.



          Comments



          // Enum representing the Length, specifier and type information provided by the format string


          seems borderline unnecessary to me. What the enums represent is already made sufficiently clear by their name, only the information bit linking them to the whole format string functionality is not given in the code (however, since the header is named FormatInfo.h, I would say this information is implied).



          // printf doc http://www.cplusplus.com/reference/cstdio/printf/
          // Use the same values as in printf documentation so it is easy to lookup
          // Use the same values as in printf documentation so it is easy to lookup
          // The type we are expecting
          // If width and/or precision are specified by parameter


          That first comment is bad, because linking to external resources is subject to link rotting. Only do so when the information that is provided there is not easily available anywhere else. In the case of printf parameters this is not the case; most POSIX-compliant systems even have a manpage explaining them thoroughly. Furthermore, the use of printf is so ubiquitous that I'd say it is acceptable to assume users are able to find documentation on their own.



          The next two comments are exactly the same and should be coerced into a single commit above the two applying enums.



          Both of the last comments do not at a whole lot of valuable information to the code, so I'd say you should omit them.



          Structure



          FormatInfo seems to contain three separate "blocks" of information/settings. Since these blocks each mark a certain category of information, I would have made nested structs for them. My suggestion would look something akin to this:



          struct FormatInfo 
          struct Flags
          bool leftJustify;
          bool forceSignWidth;
          bool prefixType;
          bool leftPad;
          ;

          struct Dimension
          std::size_t width;
          std::size_t precision;
          Length length;
          Specifier specifier;
          ;

          struct FormatSettings
          Type type;
          Dynamic useDynamicSize;
          AllowedType expectedType;
          std::ios_base::fmtflags format;
          ;

          Flags flags;
          Dimension dimension;
          FormatSettings formatSettings;
          ;


          which encapsulates the different setting groups better.



          Depending on your use case, there is also a point in making the four enums from the top of your file part of FormatInfo if this makes sense from a structural point of view (again, I don't have an overview of your whole project structure).




          Apart from the mentioned points, some points of my answer to Part 1 also apply here, for example the point about indentation.






          share|improve this answer























            Your Answer




            StackExchange.ifUsing("editor", function ()
            return StackExchange.using("mathjaxEditing", function ()
            StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix)
            StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
            );
            );
            , "mathjax-editing");

            StackExchange.ifUsing("editor", function ()
            StackExchange.using("externalEditor", function ()
            StackExchange.using("snippets", function ()
            StackExchange.snippets.init();
            );
            );
            , "code-snippets");

            StackExchange.ready(function()
            var channelOptions =
            tags: "".split(" "),
            id: "196"
            ;
            initTagRenderer("".split(" "), "".split(" "), channelOptions);

            StackExchange.using("externalEditor", function()
            // Have to fire editor after snippets, if snippets enabled
            if (StackExchange.settings.snippets.snippetsEnabled)
            StackExchange.using("snippets", function()
            createEditor();
            );

            else
            createEditor();

            );

            function createEditor()
            StackExchange.prepareEditor(
            heartbeatType: 'answer',
            convertImagesToLinks: false,
            noModals: false,
            showLowRepImageUploadWarning: true,
            reputationToPostImages: null,
            bindNavPrevention: true,
            postfix: "",
            onDemand: true,
            discardSelector: ".discard-answer"
            ,immediatelyShowMarkdownHelp:true
            );



            );








             

            draft saved


            draft discarded


















            StackExchange.ready(
            function ()
            StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f188810%2fc-string-formatter-again-part-2%23new-answer', 'question_page');

            );

            Post as a guest






























            1 Answer
            1






            active

            oldest

            votes








            1 Answer
            1






            active

            oldest

            votes









            active

            oldest

            votes






            active

            oldest

            votes








            up vote
            3
            down vote













            Very little code prompts a very small review. Here are a few things that came to my mind:



            Includes



            One issue is definitely that you are missing #include <utility> for std::pair, as well as #include <cstddef> (or similar) for std::size_t.



            Another thing that might turn out to make some people angry at you is the fact that you #include <iostream>. The reason for this is that this header injects some static initializers into the current compilation unit which initialize the special streams it provides, which in turn leads to binary bloat and a possible performance reduction (take a look at the assembly that your header generates).



            On almost all occasions, #include <iostream> can be avoided by including one of several other headers, in you case #include <ios>, which provides std::ios_base.



            Ugh, std::type_info



            Usually, seeing std::type_info around is setting off some alarm bells for me. The cause for this is not that there is anything inherently wrong with std::type_info, but that most things it could be used for are better expressed through polymorphism or compile time template switching.



            From the single file you provide, I cannot possibly tell whether that is the case with your code or not. However, passing a pointer to some std::type_info around together with an int does hint at somewhat dubious practices. Please reevaluate whether the use of std::type_info is really appropriate.



            Comments



            // Enum representing the Length, specifier and type information provided by the format string


            seems borderline unnecessary to me. What the enums represent is already made sufficiently clear by their name, only the information bit linking them to the whole format string functionality is not given in the code (however, since the header is named FormatInfo.h, I would say this information is implied).



            // printf doc http://www.cplusplus.com/reference/cstdio/printf/
            // Use the same values as in printf documentation so it is easy to lookup
            // Use the same values as in printf documentation so it is easy to lookup
            // The type we are expecting
            // If width and/or precision are specified by parameter


            That first comment is bad, because linking to external resources is subject to link rotting. Only do so when the information that is provided there is not easily available anywhere else. In the case of printf parameters this is not the case; most POSIX-compliant systems even have a manpage explaining them thoroughly. Furthermore, the use of printf is so ubiquitous that I'd say it is acceptable to assume users are able to find documentation on their own.



            The next two comments are exactly the same and should be coerced into a single commit above the two applying enums.



            Both of the last comments do not at a whole lot of valuable information to the code, so I'd say you should omit them.



            Structure



            FormatInfo seems to contain three separate "blocks" of information/settings. Since these blocks each mark a certain category of information, I would have made nested structs for them. My suggestion would look something akin to this:



            struct FormatInfo 
            struct Flags
            bool leftJustify;
            bool forceSignWidth;
            bool prefixType;
            bool leftPad;
            ;

            struct Dimension
            std::size_t width;
            std::size_t precision;
            Length length;
            Specifier specifier;
            ;

            struct FormatSettings
            Type type;
            Dynamic useDynamicSize;
            AllowedType expectedType;
            std::ios_base::fmtflags format;
            ;

            Flags flags;
            Dimension dimension;
            FormatSettings formatSettings;
            ;


            which encapsulates the different setting groups better.



            Depending on your use case, there is also a point in making the four enums from the top of your file part of FormatInfo if this makes sense from a structural point of view (again, I don't have an overview of your whole project structure).




            Apart from the mentioned points, some points of my answer to Part 1 also apply here, for example the point about indentation.






            share|improve this answer



























              up vote
              3
              down vote













              Very little code prompts a very small review. Here are a few things that came to my mind:



              Includes



              One issue is definitely that you are missing #include <utility> for std::pair, as well as #include <cstddef> (or similar) for std::size_t.



              Another thing that might turn out to make some people angry at you is the fact that you #include <iostream>. The reason for this is that this header injects some static initializers into the current compilation unit which initialize the special streams it provides, which in turn leads to binary bloat and a possible performance reduction (take a look at the assembly that your header generates).



              On almost all occasions, #include <iostream> can be avoided by including one of several other headers, in you case #include <ios>, which provides std::ios_base.



              Ugh, std::type_info



              Usually, seeing std::type_info around is setting off some alarm bells for me. The cause for this is not that there is anything inherently wrong with std::type_info, but that most things it could be used for are better expressed through polymorphism or compile time template switching.



              From the single file you provide, I cannot possibly tell whether that is the case with your code or not. However, passing a pointer to some std::type_info around together with an int does hint at somewhat dubious practices. Please reevaluate whether the use of std::type_info is really appropriate.



              Comments



              // Enum representing the Length, specifier and type information provided by the format string


              seems borderline unnecessary to me. What the enums represent is already made sufficiently clear by their name, only the information bit linking them to the whole format string functionality is not given in the code (however, since the header is named FormatInfo.h, I would say this information is implied).



              // printf doc http://www.cplusplus.com/reference/cstdio/printf/
              // Use the same values as in printf documentation so it is easy to lookup
              // Use the same values as in printf documentation so it is easy to lookup
              // The type we are expecting
              // If width and/or precision are specified by parameter


              That first comment is bad, because linking to external resources is subject to link rotting. Only do so when the information that is provided there is not easily available anywhere else. In the case of printf parameters this is not the case; most POSIX-compliant systems even have a manpage explaining them thoroughly. Furthermore, the use of printf is so ubiquitous that I'd say it is acceptable to assume users are able to find documentation on their own.



              The next two comments are exactly the same and should be coerced into a single commit above the two applying enums.



              Both of the last comments do not at a whole lot of valuable information to the code, so I'd say you should omit them.



              Structure



              FormatInfo seems to contain three separate "blocks" of information/settings. Since these blocks each mark a certain category of information, I would have made nested structs for them. My suggestion would look something akin to this:



              struct FormatInfo 
              struct Flags
              bool leftJustify;
              bool forceSignWidth;
              bool prefixType;
              bool leftPad;
              ;

              struct Dimension
              std::size_t width;
              std::size_t precision;
              Length length;
              Specifier specifier;
              ;

              struct FormatSettings
              Type type;
              Dynamic useDynamicSize;
              AllowedType expectedType;
              std::ios_base::fmtflags format;
              ;

              Flags flags;
              Dimension dimension;
              FormatSettings formatSettings;
              ;


              which encapsulates the different setting groups better.



              Depending on your use case, there is also a point in making the four enums from the top of your file part of FormatInfo if this makes sense from a structural point of view (again, I don't have an overview of your whole project structure).




              Apart from the mentioned points, some points of my answer to Part 1 also apply here, for example the point about indentation.






              share|improve this answer

























                up vote
                3
                down vote










                up vote
                3
                down vote









                Very little code prompts a very small review. Here are a few things that came to my mind:



                Includes



                One issue is definitely that you are missing #include <utility> for std::pair, as well as #include <cstddef> (or similar) for std::size_t.



                Another thing that might turn out to make some people angry at you is the fact that you #include <iostream>. The reason for this is that this header injects some static initializers into the current compilation unit which initialize the special streams it provides, which in turn leads to binary bloat and a possible performance reduction (take a look at the assembly that your header generates).



                On almost all occasions, #include <iostream> can be avoided by including one of several other headers, in you case #include <ios>, which provides std::ios_base.



                Ugh, std::type_info



                Usually, seeing std::type_info around is setting off some alarm bells for me. The cause for this is not that there is anything inherently wrong with std::type_info, but that most things it could be used for are better expressed through polymorphism or compile time template switching.



                From the single file you provide, I cannot possibly tell whether that is the case with your code or not. However, passing a pointer to some std::type_info around together with an int does hint at somewhat dubious practices. Please reevaluate whether the use of std::type_info is really appropriate.



                Comments



                // Enum representing the Length, specifier and type information provided by the format string


                seems borderline unnecessary to me. What the enums represent is already made sufficiently clear by their name, only the information bit linking them to the whole format string functionality is not given in the code (however, since the header is named FormatInfo.h, I would say this information is implied).



                // printf doc http://www.cplusplus.com/reference/cstdio/printf/
                // Use the same values as in printf documentation so it is easy to lookup
                // Use the same values as in printf documentation so it is easy to lookup
                // The type we are expecting
                // If width and/or precision are specified by parameter


                That first comment is bad, because linking to external resources is subject to link rotting. Only do so when the information that is provided there is not easily available anywhere else. In the case of printf parameters this is not the case; most POSIX-compliant systems even have a manpage explaining them thoroughly. Furthermore, the use of printf is so ubiquitous that I'd say it is acceptable to assume users are able to find documentation on their own.



                The next two comments are exactly the same and should be coerced into a single commit above the two applying enums.



                Both of the last comments do not at a whole lot of valuable information to the code, so I'd say you should omit them.



                Structure



                FormatInfo seems to contain three separate "blocks" of information/settings. Since these blocks each mark a certain category of information, I would have made nested structs for them. My suggestion would look something akin to this:



                struct FormatInfo 
                struct Flags
                bool leftJustify;
                bool forceSignWidth;
                bool prefixType;
                bool leftPad;
                ;

                struct Dimension
                std::size_t width;
                std::size_t precision;
                Length length;
                Specifier specifier;
                ;

                struct FormatSettings
                Type type;
                Dynamic useDynamicSize;
                AllowedType expectedType;
                std::ios_base::fmtflags format;
                ;

                Flags flags;
                Dimension dimension;
                FormatSettings formatSettings;
                ;


                which encapsulates the different setting groups better.



                Depending on your use case, there is also a point in making the four enums from the top of your file part of FormatInfo if this makes sense from a structural point of view (again, I don't have an overview of your whole project structure).




                Apart from the mentioned points, some points of my answer to Part 1 also apply here, for example the point about indentation.






                share|improve this answer















                Very little code prompts a very small review. Here are a few things that came to my mind:



                Includes



                One issue is definitely that you are missing #include <utility> for std::pair, as well as #include <cstddef> (or similar) for std::size_t.



                Another thing that might turn out to make some people angry at you is the fact that you #include <iostream>. The reason for this is that this header injects some static initializers into the current compilation unit which initialize the special streams it provides, which in turn leads to binary bloat and a possible performance reduction (take a look at the assembly that your header generates).



                On almost all occasions, #include <iostream> can be avoided by including one of several other headers, in you case #include <ios>, which provides std::ios_base.



                Ugh, std::type_info



                Usually, seeing std::type_info around is setting off some alarm bells for me. The cause for this is not that there is anything inherently wrong with std::type_info, but that most things it could be used for are better expressed through polymorphism or compile time template switching.



                From the single file you provide, I cannot possibly tell whether that is the case with your code or not. However, passing a pointer to some std::type_info around together with an int does hint at somewhat dubious practices. Please reevaluate whether the use of std::type_info is really appropriate.



                Comments



                // Enum representing the Length, specifier and type information provided by the format string


                seems borderline unnecessary to me. What the enums represent is already made sufficiently clear by their name, only the information bit linking them to the whole format string functionality is not given in the code (however, since the header is named FormatInfo.h, I would say this information is implied).



                // printf doc http://www.cplusplus.com/reference/cstdio/printf/
                // Use the same values as in printf documentation so it is easy to lookup
                // Use the same values as in printf documentation so it is easy to lookup
                // The type we are expecting
                // If width and/or precision are specified by parameter


                That first comment is bad, because linking to external resources is subject to link rotting. Only do so when the information that is provided there is not easily available anywhere else. In the case of printf parameters this is not the case; most POSIX-compliant systems even have a manpage explaining them thoroughly. Furthermore, the use of printf is so ubiquitous that I'd say it is acceptable to assume users are able to find documentation on their own.



                The next two comments are exactly the same and should be coerced into a single commit above the two applying enums.



                Both of the last comments do not at a whole lot of valuable information to the code, so I'd say you should omit them.



                Structure



                FormatInfo seems to contain three separate "blocks" of information/settings. Since these blocks each mark a certain category of information, I would have made nested structs for them. My suggestion would look something akin to this:



                struct FormatInfo 
                struct Flags
                bool leftJustify;
                bool forceSignWidth;
                bool prefixType;
                bool leftPad;
                ;

                struct Dimension
                std::size_t width;
                std::size_t precision;
                Length length;
                Specifier specifier;
                ;

                struct FormatSettings
                Type type;
                Dynamic useDynamicSize;
                AllowedType expectedType;
                std::ios_base::fmtflags format;
                ;

                Flags flags;
                Dimension dimension;
                FormatSettings formatSettings;
                ;


                which encapsulates the different setting groups better.



                Depending on your use case, there is also a point in making the four enums from the top of your file part of FormatInfo if this makes sense from a structural point of view (again, I don't have an overview of your whole project structure).




                Apart from the mentioned points, some points of my answer to Part 1 also apply here, for example the point about indentation.







                share|improve this answer















                share|improve this answer



                share|improve this answer








                edited Mar 6 at 20:12









                yuri

                3,3862832




                3,3862832











                answered Mar 5 at 21:02









                Ben Steffan

                4,85011234




                4,85011234






















                     

                    draft saved


                    draft discarded


























                     


                    draft saved


                    draft discarded














                    StackExchange.ready(
                    function ()
                    StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f188810%2fc-string-formatter-again-part-2%23new-answer', 'question_page');

                    );

                    Post as a guest













































































                    Popular posts from this blog

                    Python Lists

                    Aion

                    JavaScript Array Iteration Methods