c42f / tinyformat

Minimal, type safe printf replacement library for C++
531 stars 75 forks source link

Added support for format specifier represented as string object #50

Open alfishe opened 6 years ago

alfishe commented 6 years ago

Once you're working with std::string objects, if you're creating composite format specifiers, most likely they'll be already in std::string. So why not to support them? Small addition - big convenience.

c42f commented 6 years ago

This seems kind of reasonable - would you mind expanding on your use case, for example by pasting in some code so I can see what you're trying to do? It's relatively unusual to programatically control the format string, I just wonder whether there's a better way to achieve what you need, for example, just using a stringstream along with tfm::format().

Do you need this functionality habitually, or is it just in one or two places in your code?

nigels-com commented 6 years ago

Hello @c42f. One use-case I can think of is that PointTool allows for a per-attribute format string for controlling the precision of half/double/float attributes. We might want mm for x and y, but cm for AGL, for example.

lgritz commented 6 years ago

Hopefully, in a C++17 world, both of these options will go away and the format specifier will be a std::string_view, and then callers can pass anything that converts implicitly to a string_view.

alfishe commented 6 years ago

I would say that most people starting work with tfm were using plain C and printf for decades. So advising to use stringstream for them - it's like jump to the Moon immediately. So I'm talking about trivial cases like string formatter = prefix + " - %s"; tfm::format(formatter, text.c_str();

And yes, probably the next thing might be - to support objects directly (like it's done in Obj-C and other languge formatters). format("%@". stringObj). Then I would like to have standard Object.toString() like in Java and so one and so forth =)))

c42f commented 6 years ago

string formatter = prefix + " - %s"; tfm::format(formatter, text.c_str();

If you've got simple cases like that, better to just use the format string:

std::string prefix = "a prefix";
std::string text = "some text";
std::string out = tfm::format("%s -%s", prefix, text);

Regarding functionality such as Java's toString() - tinyformat already has this functionality. Your object just needs to support the standard operator<<(std::ostream&, const MyObj& obj). You can use it with any of the format conversions, but I suggest just using %s unless you've got a composite numeric type (such as a short vector) where the %f conversions can be very useful.

The reason I'm digging for use cases here is I have a niggling feeling that using format strings which aren't string literals is a fairly good sign that you're doing something unusual - something which would probably better be done a different way. For this reason I'm still reluctant to merge this change - I feel it might be a good thing to force people to write c_str() as a sign that they're using the API in an unnatural way. In the relatively rare cases that this is quite intentional, a little extra syntax (ie, calling c_str()) doesn't hurt too much.

nigels-com commented 6 years ago

@c42f The main argument I'd offer in favour of std::string format string is that it seems odd in C++ to prefer a bare char pointer, in a context that richly supports std::string. I'd tend to think of c_str() as something you'd do for a C API. I do recall contemplating a nested tfm:format for forming the formatter for an outer tfm::format, but the complexity of that is generally unappealing to me, with or without the c_str() thrown in.

c42f commented 6 years ago

a per-attribute format string for controlling the precision of half/double/float attributes

This is definitely useful functionality, I think it makes sense in restricted circumstances like you have (ie, formatting exactly one value per format string).

In general taking format strings from the user is a bit of an oddity: They're allowed to put in any possibly invalid string, and this has to match to the list of arguments which are hardcoded in the source. So there's an uncomfortable coupling between user entered information and the source code. Plain printf would be a security/usability disaster for this case... tinyformat is ok-ish as it can be made to throw an exception, but the uncomfortable coupling remains.

c42f commented 6 years ago

I'd tend to think of c_str() as something you'd do for a C API

Sure. I still feel that tolerating this uglyness is a net win, as it's (more often than not) a sign that people are "doing it wrong". Here's another classic example from printf land:

const char* message = /* get a message from somewhere */;
printf(message); // Wrong!
printf("%s", message); // Correct

The point here is that if your message comes from an arbitrary source, it can easily contain formatting conversions, which will cause the program to crash. Therefore, you need the redundant-looking conversion %s. This example also holds for tinyformat, but at least it won't crash the application or execute arbitrary code.

nigels-com commented 6 years ago

There is a principle of least surprise to consider too.

In production coding you'd likely get some long logs of generally unhelpful compilation errors. Perhaps at least we could statically assert for std::string to indicate the recommended usage.