attempt to provide the original format string during errors

[gcflymoto]

I tried to create better error messages when the format library throws errors by including the original format string. The motivation for this is when there are 100s of format strings it's not fast to determine which format string is broken.

This first attempt causes undefined symbols during runtime. It's also incomplete, I have different version and found out through testing that the format string is NULL at the point of printing it out during the error.

Looking for suggestions on how to make this work.

[vitaut]

Formatting string is relevant only to BasicFormatter, but not to BasicWriter. Therefore I suggest not introducing the format string to BasicWriter and related classes such as FormatSpec, but rather translate the FormatError exception thrown by BasicWriter in BasicFormatter::DoFormat adding the information about the format string, something along the lines of:

    void fmt::BasicFormatter<Char>::DoFormat() {
      const char *format = format_; // capture the format string before it is reset
      ...
      try {
        // Format argument.
        switch (arg.type) {
        case INT:
          writer.FormatInt(arg.int_value, spec);
          break;
        ...
        }
      } catch (const FormatError &e) {
        // rethrow FormatError with the format string pointed to by format
      }
    }

[gcflymoto]

Two problems with this.

1. The original is a capital Char type,
   const Char *start= format_;
   there are all kinds of compile errors with
   const char *start= format_;
   since its trying to convert wchar_t to char
2. I tried to rethrow it as
   throw FormatError(fmt::c_str(fmt::Format("error: {} while parsing {}") << e.what() << start));

But of course start can be of type const wtype_t char* and Arg() doesn't allow it

error: 'fmt::BasicFormatter::Arg::Arg(const T*) [with T = wchar_t, Char = char]' is private

[vitaut]

Wide strings are a problem because to format a wide (wchar_t) string as a char string one needs to (1) know both encodings and (2) a char string should be able to represent all the characters from the charset used by wchar_t which is not possible in general. One possible solution is to return the format string as a separate member of exception:

    template <typename Char>
    class BasicFormatError : public std::runtime_error {
     private:
      std::basic_string<Char> format_;
     public:
      explicit BasicFormatError(const std::string &message, const Char *format)
      : std::runtime_error(message), format_(format) {}
      const Char *format() const { return format_.c_str(); }
    };

Then anyone interested in the format string can access it either as a char string or as a wchar_t string (normally an application uses only one of these). This also solves the issue of an error message being too large because of including the format string.

[gcflymoto]

Ok, I followed your advice. The pull request got lumped together with the unsigned long long. Sorry. Is it easier to do these as separate branches?

In any case, it finally works. 'char' type format error message show their original format message. I didnt bother with 'wchar_t'

[vitaut]

Yes, separate branches would be better.

I think that having different error messages for char and wchar_t strings is quite confusing. I'd leave the format string out of the error message in both cases. If anyone is interested in the format string they can easily access it using the format() method of the exception object. This should rarely be necessary because formatting errors should be caught during testing and one can always see the source of the exception from the stack trace. Python's str.format, this library is modeled after, doesn't print the format string either.

Thanks for working on this.

[gcflymoto]

The project I work on has a global exception catcher, which cannot be modified for this specific library, or for any other library/code that might throw library specific exceptions. Because of this there is no stack trace to see. There is no simple way to determine the location of the original throw. The latest GDB is not able to "catch exception" for some reason. So we are stuck between a rock and a hard place, and so a more informative error message is our simplest way debug defense. As far as catching errors during testing. This library is used in our testing environment which is always in production and runs for over 3 years, and because our testing relies on randomness and has a scope of over 1MLOC, it is literally impossible to get 100% coverage on our testing code. We work hard to get close to 100% coverage on the product our testing code is trying to validate. So I don't mind having my own fork which includes the more informative error message thrown by the exceptions. PS. the wchar_t to char conversation is possible, just didnt get around to it.

[vitaut]

I'm OK with adding this feature provided that it is controlled by the configuration option and the default behavior remains the same (error messages not including the format string).

[gcflymoto]

what kind of configuration option are we talking about? #ifdef or something else?

[gcflymoto]

I would prefer #ifdef and not change the formats library api. Another option is to set a static class variable in one of format's classes

[vitaut]

Yes, #ifdef and a CMake option to control it, something like:

CMakeLists.txt:

option(FORMAT_STRING_IN_ERRORS "Report format string in errors" OFF)
if (FORMAT_STRING_IN_ERRORS)
  add_definitions(-DFMT_FORMAT_STRING_IN_ERRORS=1)
endif ()

Then FMT_FORMAT_STRING_IN_ERRORS can be used in #ifdef in the source code.

[vitaut]

Could you please make sure that the PR only contains relevant changes and merges without conflicts.

[gcflymoto]

I've tried updating to the latest version of vitaut/format but it has compile errors

#12

[gcflymoto]

I'll try to update an use a new version of GCC

[vitaut]

The compile errors should be fixed now. The PR still has some conflicts.

[gcflymoto]

latest version merged with head and added support for original format string. Works with GCC4.2.2, 4.8.1, 4.8.1 c++11, and ICC13

[vitaut]

Thanks for checking that it works with several compilers. Regarding the current PR, GitHub still complains about the conflicts, see the note "We can’t automatically merge this pull request." below.

[gcflymoto]

sigh. alright. Ill try to break this up into two. first copy the HEAD version into the branch and then my changes.

[gcflymoto]

actually let me try it on a new branch

[vitaut]

Closing until the conflicts are resolved. See also #13 (comment)