1. 11
  1.  

  2. 8

    Excellent library, thank you for sharing! I have some feedback if you’re interested:

    You have a bug in your long long formatting.

    ggprint("{}\n", ~0ULL);
    printf("%llu\n", ~0ULL);
    
    4294967295
    18446744073709551615
    

    Your int_helper doesn’t add hh, h, l, or ll to the format string, so everything is treated as int always. Thus only half the long long is formatted. The shorter types still work properly by accident of calling convention.


    Your ggformat_impl function recursively calls itself consuming one arg at a time, and since you’re handling varargs as templates, ggformat_impl code is generated for every arg. For example ggprint("{}{}{}", 1, 1, 1); will generate individual functions that call each other in a chain, like so:

    void ggformat_impl(FormatBuffer*, char const*, int const&, int, int)
    void ggformat_impl(FormatBuffer*, char const*, int const&, int)
    void ggformat_impl(FormatBuffer*, char const*, int const&)
    

    This dramatically increases code size. This following code compiles to a 17k object file, while the corresponding printf .o is 824 bytes (pretty much just pushes and a call).

    int main() {
        ggprint("{}{}{}{}{}{}{}{}{}{}{}{}{}{}{}{}{}{}{}{}{}{}",
            1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1);
        return 0;
    }
    

    // this is UB for signed types, but who cares?

    Indeed, who cares? Though if you wanted you could fix it by casting to unsigned using std::make_unsigned<T>::type.

    1. 1

      Thanks for the feedback!

      You have a bug in your long long formatting.

      I remember thinking I should double check that… fixed now.

      Your ggformat_impl function recursively calls itself consuming one arg at a time, and since you’re handling varargs as templates, ggformat_impl code is generated for every arg.

      I’m not sure there’s anything I can do about this unfortunately. It’s also why I have the macro to disable optimisations.

      Though if you wanted you could fix it by casting to unsigned using std::make_unsigned::type.

      Avoiding STL includes is a big part of keeping compile times down, and adding a wall of

      static unsigned char make_unsigned( signed char x ) { return x; }
      static unsigned char make_unsigned( unsigned char x ) { return x; }
      // etc
      

      to work around something that’s very unlikely to actually matter isn’t worth it IMO.

      1. 2

        I’m not sure there’s anything I can do about this unfortunately. It’s also why I have the macro to disable optimisations.

        Optimizations make the code smaller for me on Clang. There is no code in that function that would be expanded by any reasonable optimizer.

        As for what you can do, you can do pack expansion. That would result in one function per argument list, which is still a lot but it would be less.

        to work around something that’s very unlikely to actually matter isn’t worth it IMO.

        I agree.

        1. 1

          There is no code in that function that would be expanded by any reasonable optimizer.

          Sorry I wasn’t clear - I disable optimisations so the compiler doesn’t waste time trying to optimise the huge piles of junk code. Runtime performance isn’t a priority for me, and it makes quite a difference at compile time. On this machine building basic_examples.cc at -O2 with the pragmas commented out takes 40% longer!

          As for what you can do, you can do pack expansion. That would result in one function per argument list, which is still a lot but it would be less.

          AFAIK you can’t directly iterate over pack expansions though.

          1. 1

            You can, by pack expanding into an array and iterating over that.

            1. 1

              That won’t work if the arguments aren’t all of the same time (ggprint( "{} {}\n", 1, 1.0 );). At least not without some implicit constructor trickery, which I think would get in the way of supporting user defined types.

              1. 1

                The constructor just has to invoke format. It’s not pretty but it works. I’d write some sample code except I actually have to work on my real job. ?

    2. 3

      Nice! Are you sure you don’t want to give an easier way of setting FPs for malloc/free?

      1. 1

        There is exactly one call to malloc, and exactly one corresponding call to free. Pretty straightforward.

        1. 3

          Not the point. Typically, good libraries in C/C++ allowed setting of callbacks for things like logging, memory allocation, and file access.

          It’s a little annoying to keep your own patched version of a library even if it is just one call.

        2. 1

          The library is feature complete so once the initial bugs are ironed out I expect to rarely if ever update this code again, and like Peter said it’s just one pair of malloc/free. In this case I think “ctrl+f for malloc and replace it with whatever” is easier than “figure out what my mechanism for replacing malloc is, then write some shim code so your allocator API matches what I expect”.

        3. 1

          I hope the author doesn’t take this personally, but browsing the GitHub repo for a few minutes I see a lot of problems with this library that would make me not use it.

          • The entire interface uses FILE* and char* all over the place.
          • There’s no support for std::string at all, much less newer features like string views
          • ggformat.cc is including old-school *.h headers, including stdlib.h, string.h (the C library one…).
          • string_examples.cc defines its own string template class “str”.

          In short, it’s terrible C++.

          I’d strongly suggest anybody looking to use this in a project take a look at some of the alternatives, like boost::format. At the very least make sure to run valgrind if you use this one.

          1. 2

            I’ve intentionally avoided the C++ STL because the STL headers are enormous and really hurt compile times, and there are certain fields (games) where using the STL (especially std::string!) is not considered cool.

            That said, it’s easy enough to integrate:

            template< typename... Rest >
            std::string ggformat_to_string( const char * fmt, Rest... rest ) {
                    size_t space_required = ggformat( NULL, 0, fmt, rest... );
            
                    std::string result;
                    result.resize( space_required + 1 ); // + 1 so there's space for the null terminator...
                    ggformat( &result[ 0 ], space_required + 1, fmt, rest... );
                    result.resize( space_required ); // ...and then trim it off
            
                    return result;
            }
            

            and I’ll add that to the README also.

            valgrind

            ggformat passes memcheck, and once ASAN gets fixed I will test with that too.