1

I am trying to modify my log class to accept variables in my string. For example, if I wanted to output that there are 7 players in an area.

Here is my write to log function:

void Log::writeSuccess(string text,...)
{
    // Write the sucessfull operation to the logfile
    logfile << "<---> " << text << endl;
}

And here is my calling code:

int playernum = 7;

errorLog.writeSuccess("There are %i players in the area", playernum);

It just ends up outputting to the file: There are %i players in the area

Any way to fix this?

Improve this question
8
  • 2
    I'm surprised this doesn't cause a compiler error, since you are passing the wrong number of arguments. The answer to your question is probably boost.log or boost.format, or perhaps variadic templates if you have C++0x. Commented Jul 27, 2011 at 11:30
  • 2
    You expected magic to happen? Commented Jul 27, 2011 at 11:31
  • 3
    This is a logging library and you're passing std::string by value? Commented Jul 27, 2011 at 12:02
  • 1
    @sbi instead of just acting surprised, explain to me please why this is unwise... Commented Jul 27, 2011 at 12:13
  • 2
    @Publeus: No offense meant, but why are you tasked with doing this when you don't know about passing arguments? Is this homework? A job? A hobby project? Commented Jul 27, 2011 at 13:10

5 Answers 5

Reset to default
6

I wonder how on earth does your program even compile?! You call writeSuccess with 2 arguments, whereas it is declared to take only one argument.

You should look at boost format

Improve this answer
Sign up to request clarification or add additional context in comments.

3 Comments

I bet he hasn't compiled it yet :)
Sorry, was fiddling with it. I reinstated what I had compiling before I posted.
I can't see why this still gets upvoted. It obviously doesn't apply to the code in the question anymore. Don't you guys even look at that before you vote?
2

The problem with using printf-style format strings is that those strings are

  1. dependent on the types of the provided arguments, and
  2. dependent on the order of the provided arguments.

Not only is this error-prone when you are writing those lines. In my experience the types and order of the arguments will easily change in software that is actively maintained and extended, and it's much harder still to keep the format strings in sync with changes applied later, than it is to do so when you initially write the code.

The problem of needing to manually keep the parameter types in sync with the format string can easily be solved in C++, streams have proven that 25 years ago. Boost.Format even manages to combine format strings with type safety.

A different approach, solving both problems, is taken by some logging libraries I have seen: They use a syntax where you specify which parameter is to be inserted at a specific place in a string by using the parameter's name, and they free you from having to think about the parameter's type by individually converting all parameters to strings before inserting them: 

log( "i now has the value of @(i), current size is @(x.get_size(y))", 
     LOG_PARAM(i) + LOG_PARAM(x.get_size(y)) );
Improve this answer

Comments

1

If you don't want to use stdarg.h which doesn't look good in c++ IMO. you can do something like this. Keep in mind that although this is a small class (you can add to it for better logging), its not the most efficient way to do it. 

#include <iostream>
#include <sstream>


class Log
{
public:
    Log() : os()
    {

    }

    ~Log()
    {
        fprintf(stderr, "%s\n", os.str().c_str());
    }

    template<typename T>
    std::ostringstream &operator<<(const T &t)
    {
        os << "Log file - " << t;
        return os;
    }

private:
    std::ostringstream os;
};

int main(int argc, char *argv[])
{
    //usage
    for (int i = 0; i < 10; ++i)
        Log() << "Hello world " << i;

    return 0;
}
Improve this answer

Comments

0

Look at stdarg standard library. It allows you to handle variable number of parameters.

Improve this answer

1 Comment

It's a solution used by printf and the like, but it's notoriously type-unsafe. IMO boost format is better
-7

In case you can't or won't use boost:

void Log::writeSuccess(const char* const fmt, ...) {
    va_list ap;
    va_start(ap, fmt);
    char buff[1024];
    vsnprintf(buff, sizeof(buff), fmt, ap);
    logfile << buff;
}

Note: it assumes that the written length is limited.

Update: with gcc it's possible to do this in a type-safe way, you need the following declaration.

class Log {
    void writeSuccess(const char* const fmt, ...) __attribute__ ((format (printf, 2, 3)));
    //...
};

Info here. Note: it's a warning, not a compile error. If you ignore warnings that's your problem..:)

Improve this answer

25 Comments

Magic numbers make puppies CRY with how horrifically bad they are. Do not ever write code like this.
red alert, red alert, magic number in a simple illustrating example sigh
The "magic number" refers to the 1024 in char buff[1024]. This places a hard-coded maximum length on the string that can be output, but nothing prevents a longer string being passed as a parameter.
vsnprintf is not type-safe and shouldn't be used in C++, we have much better alternatives.
@yi_H: You should never illustrate bad practice. Expecting the questioner to fill in a few blanks for themselves is one thing, but illustrating things which are completely and utterly wrong is another. You should compute the space required and allocate it dynamically, then output that.
|

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.