6
\$\begingroup\$

I'm hoping to get some feedback on a function with variadic templates that parses a format string and fills in some parameters whose order and types are based on the characters in the format string. Kind of like scanf().

(For the curious, I am trying to make a typesafe version of this function, documentation here.)

I'm pretty experienced with C but the last time I did anything serious with C++ was at university 15 years ago and it seems like a completely different language now.

This toy code works for me and fails in the ways I'd expect it to when I give it bad input. However, I'm not too familiar with the standard library so I have a strong feeling that I may be missing something. Also the code seems overly verbose for what it does.

So here's what I'm hoping to find out:

  • Is this code idiomatic modern C++?
  • Am I overlooking useful features in the standard library that would make things easier?
  • Should I be using pointers in this way, or am I thinking too much in C?
  • Should I consider another approach entirely, like tscanf(format, paramname1, paramname2) >> param1 >> param2?

I am constrained to use C++11, the code currently does not use Boost.

#include <cstring>
#include <iostream>

struct SomeStruct {
    int val;
};

template<typename T>
bool
assign(const char c, T& ref) {
    return false;
}

template<>
bool
assign(const char c, bool*& ref) {
    if (c != 'b')
        return false;
    *ref = true;
    return true;
}

template<>
bool
assign(const char c, int*& ref) {
    if (c != 'i')
        return false;
    *ref = 42;
    return true;
}

template<>
bool
assign(const char c, SomeStruct& ref) {
    if (c != 'h')
        return false;
    ref.val = 81;
    return true;
}

template<typename T>
bool
tscanf(const char *format, const char *name, T& ref)
{
    if (std::strlen(format) != 1) {
        std::cerr << "Wrong number of arguments" << std::endl;
        return false;
    }

    if (!assign(format[0], ref)) {
        std::cerr << "Wrong type for argument " << name << std::endl;
        return false;
    }

    return true;
}

template<typename T, typename... Args>
bool
tscanf(const char *format, const char *name, T& ref, Args... args)
{
    if (!assign(format[0], ref)) {
        std::cerr << "Wrong type for argument " << name << std::endl;
        return false;
    }

    return tscanf(format + 1, args...);
}

int
main(void)
{
    bool b;
    int i;
    SomeStruct h;
    if (!tscanf("hbi",
        "handle", h,
        "switch", &b,
        "num", &i))
        return 1;
    std::cout << h.val << ' ' << b  << ' ' << i << std::endl;
    return 0;
}
\$\endgroup\$
  • \$\begingroup\$ Have you thought about what C# did? I think the {N} notation is very useful, since it's no longer needed to check for static type and runtime (the things in string) to match, which will untie the hands of the compiler. On top of that, the caller will now only need to worry about one sequence, not both. \$\endgroup\$ – Incomputable Oct 12 '16 at 11:11
4
\$\begingroup\$

So here's what I'm hoping to find out: Is this code idiomatic modern C++?

Pretty much.
You use recursion to handle the multiple arguments. But a better technique may be to use the compiler to build objects to handle each argument. Unfortunately the way you have to defined the interface makes this a bit hard. If we re-consider the interface to make it like C-scanf.

template<typename T>
int parser(char const*& format, char const*& value, T& value)
{
     // Parse format and value.
     // convert the first % in format into value and save in value.
     // return 1 if value was correctly parsed otherwise 0
}

template<typename... Args)
int tscanf(char const* format, char const* value, Args&... args)
{
    // This will call parser() for each argument.
    // Each of the parameters inside {} are guaranteed to be constructed
    // left to right.
    // Because the compiler knows the number of arguments you
    // get a fixed size array that can be used to count the
    // number of parameters that were actually found.
    auto count[] = {parser(format, value, args)...};

    // Probably need a check for unmatched format parts.

    // The return the number of format parameters matched.
    return std::accumulate(std::begin(count), std::end(count), 0);
}

Am I overlooking useful features in the standard library that would make things easier?

I don't think there is anything you absolutely need.

Should I be using pointers in this way, or am I thinking too much in C?

NO. Absolutely do not use pointers like that.

Prefer to use references. This way you don't need to check for null. You also express your intent much more clearly so there is no confusion on the ownership of the pointer.

Should I consider another approach entirely, like tscanf(format, paramname1, paramname2) >> param1 >> param2?

There have been a couple of attempts like this. boost::format springs to mind. They used % rather than >>. But personally I don't see a need for this. BUT big advancements have come from trying the non obvious (things I would not think of). So please experiment.

\$\endgroup\$
  • \$\begingroup\$ I believe that Boost.Format decided to overload operator % because there were no variadic templates at the time it was designed. \$\endgroup\$ – 5gon12eder Oct 8 '16 at 21:55
  • \$\begingroup\$ @5gon12eder: The same could be said for operator>> \$\endgroup\$ – Martin York Oct 9 '16 at 4:33
  • \$\begingroup\$ @Olzhas: Why would that lead to UB? Can you create a gist with an example? \$\endgroup\$ – Martin York Oct 9 '16 at 4:34
  • \$\begingroup\$ I've removed all of the comments and tried to leave only important things. Here is the real gist on gist.github. I'm sorry for inconvenience. \$\endgroup\$ – Incomputable Oct 9 '16 at 4:57
  • \$\begingroup\$ @Olzhas: Yes the cv qualifier should be preserved. This will result in a compile time error if you try and modify it. Which is what you want. Here is a running example. gist.github.com/Loki-Astari/fe99d3dc95cd9f8ffdcd387308ca4ae7 If you uncomment the calls that pass const values then the compile will fail. So there is no UB because the compiler detects and prevents this situation. \$\endgroup\$ – Martin York Oct 9 '16 at 5:37
1
\$\begingroup\$

Bug

There is at least one bug I noticed: if format is shorter than the number of arguments minus 1, you'll only detect this when scanning the last one, where format will point past the end of string.

Interface

Output parameters

The tscanf interface is not uniform for all supported types. Some arguments (bool and int) are passed by pointer, while other types (SomeStruct and everything else) are passed by reference. The user will find this confusing.

Some style guides (most notably Google C++ Style Guide) recommend using pointers for output arguments, since it is more explicit at the call site these arguments get modified inside the function. However, whatever you chose, use for all types.

Return and error reporting

The tscanf return a boolean indicating if the scan was successful. If you want to make it more compatible with scanf, it should return the number of correctly parsed arguments. However, since the format and parameters are not scanf-like, maybe it doesn't make too much sense to follow the return of scanf.

Since this is C++, not C, handling errors could be accomplished through exceptios. Consider throwing std::invalid_argument (<stdexcept>) or something like this instead of returning false.

Also, I'd expect a function like this to not write to any output stream (cerr in this case), but let the function user to report the error in the most appropriate form for the application.

Implementation

String length

Using std::strlen to check if the string has only one character is not very efficient, since the string could be very long. You could just check if the next character is '\0'.

In order to do this and also fix the bug above, I'd suggest to verify if format[0] != '\0' before all calls to the next tscanf and in the base of recursion to check also for format[1] != '\0' instead of calling tscanf.

Base of recursion

The code will become slightly more simple and less duplicated if the base of recursion is for zero arguments, instead of one:

bool
tscanf(const char* format)
{
    return format[0] != '\0';  // or throw...
}

This way, the case for one argument is the same for multiple arguments and you'll always look at the first character of format, even to detect if the string length is correct.

The only thing that will change for the user is that tscanf(str) will work (and only assert str has zero length), while your current version doesn't compile in this case.

Function template specialization

The assign function uses function template specialization, but that is uncommon (there are rare cases where it is useful) and not needed here. The usual way to specialize a function for different types of arguments is through overloading. The multiple assign functions could be declared this way:

template<typename T> bool assign(const char c, T& ref);
bool assign(const char c, bool* ref);
bool assign(const char c, int* ref);
bool assign(const char c, SomeStruct& ref);

This is already done correctly for tscanf function: both versions are template, but one is not a template specialization of the other.

Questions

  • Is this code idiomatic modern C++?

    I think it is. Unless maybe for returning a boolean to signal errors instead of exceptions.

  • Am I overlooking useful features in the standard library that would make things easier?

    I'm not aware of any standard library feature that could help in this case.

  • Should I be using pointers in this way, or am I thinking too much in C?

    Pointers are part of C++ too and I don't see any case where they are being used without a good reason. Just avoid mixing pointers and references for similar things when both are right, as I said before.

  • Should I consider another approach entirely, like tscanf(format, paramname1, paramname2) >> param1 >> param2?

    That is very subjective and I don't have any strong opinion about it. I particularly don't like the << and >> syntax from streams. In tscanf case, this would place the names away from the variables, which I consider worse. Maybe passing format string characters separately, each one just before the parameter name, may be better.

\$\endgroup\$
  • 3
    \$\begingroup\$ Pointers for output parameters. Again the Google style guide proves it is a terrible style guide for good C++. \$\endgroup\$ – Martin York Oct 8 '16 at 19:07
  • \$\begingroup\$ Exceptions are good but not always appropriate. Even the standard stream error handling is done via an error state and it takes extra effort to make streams use exceptions. This is because streams (especially those that involve human input) tend to generate errors and handling them immediately is easier to do with error state. \$\endgroup\$ – Martin York Oct 8 '16 at 19:11
  • \$\begingroup\$ Thanks for the suggestions! The cerr writing is just to illustrate error reporting in the toy code, I wouldn't be using it in the real implementation. \$\endgroup\$ – ptomato Oct 9 '16 at 22:39
  • \$\begingroup\$ A question about pointers vs. references; in the toy code SomeStruct stands in for JS::MutableHandle<T> & which is created from a JS::Rooted<T> using the & operator, so the API would be less confusing because I would be passing in every parameter in the variable argument list with a & in front. Do you think that would justify mixing pointers and references here? Otherwise I'd have different rules per type about whether to use & on it or not. \$\endgroup\$ – ptomato Oct 9 '16 at 22:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.