Skip to content

Review: tinyformat fixes#328

Merged
lgritz merged 2 commits intoAcademySoftwareFoundation:masterfrom
lgritz:lg-tinyformat
Apr 18, 2012
Merged

Review: tinyformat fixes#328
lgritz merged 2 commits intoAcademySoftwareFoundation:masterfrom
lgritz:lg-tinyformat

Conversation

@lgritz
Copy link
Copy Markdown
Collaborator

@lgritz lgritz commented Apr 18, 2012

Needed some other tweaks to work well with OSL (and presumably other clients).

c42f and others added 2 commits April 18, 2012 13:25
All varargs error() functions are replaced here by an equivalent
function using tfm::format to do the formatting instead.  An effort has
been made to keep implementation details out of the headers by
introducing private non-formatting functions like
ImageInput::append_error() to be called by error().

tfm::format has been brought into the Strutil namespace to replace
Strutil::format().  Strutil::vformat() remains as-is, because it is
useful in certain circumstances where external libraries pass a va_list.

The error formatters inside ErrorHandler have not been changed yet,
since it's not quite clear what to replace the va_list functions like
ErrorHandler::vError() with.
@fpsunflower
Copy link
Copy Markdown
Contributor

LGTM

@lgritz lgritz merged commit 77aebbd into AcademySoftwareFoundation:master Apr 18, 2012
@c42f
Copy link
Copy Markdown
Contributor

c42f commented Apr 19, 2012

On Thu, Apr 19, 2012 at 8:03 AM, Larry Gritz
reply@reply.github.com
wrote:

Needed some other tweaks to work well with OSL (and presumably other clients).

There's one thing I don't understand about your use of
TINYFORMAT_WRAP_FORMAT here Larry: why the static? The expression

TINYFORMAT_WRAP_FORMAT (static std::string, format, /**/,
std::ostringstream msg;, msg, return msg.str();)

Expands to a set of functions in which there is two distinct kinds of
things. The no-args version is

inline static std::string format(const char* fmt)
{
    std::ostringstream msg;
    tinyformat::format(msg, fmt);
    return msg.str();
}

the verions with more than one argument are templates:

template<typename T1>
static std::string format(const char* fmt, const T1& v1)
{
    std::ostringstream msg;
    tinyformat::format(msg, fmt, v1);
    return msg.str();
}

What I'd like to understand is why these the static keyword is
reqired: I would have thought "inline static" is a contradiction
(inline and static linkage are just different, and mutually
incompatible); template functions have inline linkage by default so I
guess declaring them static might be meaningful, but I don't
understand the need.

~Chris

@lgritz
Copy link
Copy Markdown
Collaborator Author

lgritz commented Apr 19, 2012

Mistake?

Chris Foster reply@reply.github.com wrote:

On Thu, Apr 19, 2012 at 8:03 AM, Larry Gritz
reply@reply.github.com
wrote:

Needed some other tweaks to work well with OSL (and presumably other
clients).

There's one thing I don't understand about your use of
TINYFORMAT_WRAP_FORMAT here Larry: why the static? The expression

TINYFORMAT_WRAP_FORMAT (static std::string, format, /**/,
std::ostringstream msg;, msg, return msg.str();)

Expands to a set of functions in which there is two distinct kinds of
things. The no-args version is

inline static std::string format(const char* fmt)
{
std::ostringstream msg;
tinyformat::format(msg, fmt);
return msg.str();
}

the verions with more than one argument are templates:

template
static std::string format(const char* fmt, const T1& v1)
{
std::ostringstream msg;
tinyformat::format(msg, fmt, v1);
return msg.str();
}

What I'd like to understand is why these the static keyword is
reqired: I would have thought "inline static" is a contradiction
(inline and static linkage are just different, and mutually
incompatible); template functions have inline linkage by default so I
guess declaring them static might be meaningful, but I don't
understand the need.

~Chris


Reply to this email directly or view it on GitHub:
#328 (comment)

Larry Gritz
lg@larrygritz.com

@c42f
Copy link
Copy Markdown
Contributor

c42f commented Apr 19, 2012

I was assuming there was something very subtle concerning linkage which I didn't understand :)

Ah, maybe I see what happened - did you copy the usage from ustring.h ? I made the same mistake there when I was converting the static ustring::format function.

Hold on, I'll make a little patch to fix both occurrences of this oddity.

@lgritz
Copy link
Copy Markdown
Collaborator Author

lgritz commented Apr 19, 2012

Yes, that's exactly what happened.

Chris Foster reply@reply.github.com wrote:

I was assuming there was something very subtle concerning linkage which
I didn't understand :)

Ah, maybe I see what happened - did you copy the usage from ustring.h ?
I made the same mistake there when I was converting the static
ustring::format function.

Hold on, I'll make a little patch to fix both occurrences of this
oddity.


Reply to this email directly or view it on GitHub:
#328 (comment)

Larry Gritz
lg@larrygritz.com

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.

3 participants