Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for format specifier represented as string object #50

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alfishe
Copy link

@alfishe alfishe commented Dec 23, 2017

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
Copy link
Owner

c42f commented Dec 28, 2017

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
Copy link
Contributor

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
Copy link
Contributor

lgritz commented Dec 28, 2017

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
Copy link
Author

alfishe commented Dec 28, 2017

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
Copy link
Owner

c42f commented Dec 29, 2017

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
Copy link
Contributor

@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
Copy link
Owner

c42f commented Dec 29, 2017

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
Copy link
Owner

c42f commented Dec 29, 2017

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants