Skip to main content
added 16 characters in body
Source Link
Davislor
  • 8.1k
  • 17
  • 36

Support Logging to any std::ostream

Currently, you only log to stdout from the C standard library. It would be better to tell the logger which file or stream to output to when it is constructed, defaulting to std::cerr or perhaps std::clog. You can instead construct a logger that logs to a file, by passing it a std::ofstream&.

Use C++ Style I/O, or std::format

Currently, the interface is a C-style format string and variadic arguments, but without the checks the compiler does for correctness. This is obsolete and finicky.

If you accept a std::string_view, you can pass in any string-like object, including the return value of std::format. You probably want an additional overload for const char*, to cover all your bases. It also guarantees that each log entry is a single output call, which (as G. Sliepen reminded me), is already thread-safe.

Alternatively, you can overload operator<< to accept any type that is overloaded to output to an ostream&.

Use std::source_location Instead of C-style Macros

A better solution than a C-style macro for passing the logger the source line that calls it is std::source_location. Give the logging function a default final argument const std::source_location srcl = std::source_location::current(). Then, the compiler will default to passing the function the source location of the call site.

Also Replace the Other C-Style Macros

In general, use macros only when you have to. A good solution here would be to replace the macros that do formatting with wrappers that return a std::string object from std::format or std::stringstream, and pass (or maybe move) it to the logger’s output function in a tail-call. The string will automatically be destroyed when the logger is done with it and returns.

Use the Mutex

Having the threads claim a mutex to be able to log to the stream is a great idea, but threads need to actually lock the mutex before doing I/O.

If you are writing to a stream that something other than the log object might write to, it could be worthwhile to pass the constructor a reference to the mutex it should be using. That way, other functions can also lock the same mutex and interoperate with the logger. By default, the logger can still create its own mutex, which only it uses.

Are You Sure About the Private and Deleted Constructors?

There doesn’t appear to be a way to actually create one of these. If it’s meant to be a base class, the private: constructors should be protected:, so daughter classes can implicitly call them, and also should have a protected:, virtual destructor. If this is meant to be instantiated, it needs at least one public: constructor. It probably makes sense for there to be a default constructor as well, since there are sensible defaults.

Consider as well whether you actually want to delete the copy constructor. You say that you want this to be used from multiple threads, and although they might all access it through a std::shared_ptr or reference, you might legitimately want to pass each thread its own copy. That way, the object only needs to serialize the output operation itself, not all access from any of the threads.

Support Logging to any std::ostream

Currently, you only log to stdout from the C standard library. It would be better to tell the logger which file or stream to output to when it is constructed, defaulting to std::cerr or perhaps std::clog. You can instead construct a logger that logs to a file, by passing it a std::ofstream&.

Use C++ Style I/O, or std::format

Currently, the interface is a C-style format string and variadic arguments, but without the checks the compiler does for correctness. This is obsolete and finicky.

If you accept a std::string_view, you can pass in any string-like object, including the return value of std::format. You probably want an additional overload for const char*, to cover all your bases. It also guarantees that each log entry is a single output call, which (as G. Sliepen reminded me), is already thread-safe.

Alternatively, you can overload operator<< to accept any type that is overloaded to output to an ostream&.

Use std::source_location Instead of C-style Macros

A better solution than a C-style macro for passing the logger the source line that calls it is std::source_location. Give the logging function a default final argument const std::source_location srcl = std::source_location::current(). Then, the compiler will default to passing the function the source location of the call site.

Also Replace the Other C-Style Macros

In general, use macros only when you have to. A good solution here would be to replace the macros that do formatting with wrappers that return a std::string object from std::format or std::stringstream, and pass it to the logger’s output function in a tail-call. The string will automatically be destroyed when the logger is done with it and returns.

Use the Mutex

Having the threads claim a mutex to be able to log to the stream is a great idea, but threads need to actually lock the mutex before doing I/O.

If you are writing to a stream that something other than the log object might write to, it could be worthwhile to pass the constructor a reference to the mutex it should be using. That way, other functions can also lock the same mutex and interoperate with the logger. By default, the logger can still create its own mutex, which only it uses.

Are You Sure About the Private and Deleted Constructors?

There doesn’t appear to be a way to actually create one of these. If it’s meant to be a base class, the private: constructors should be protected:, so daughter classes can implicitly call them, and also should have a protected:, virtual destructor. If this is meant to be instantiated, it needs at least one public: constructor. It probably makes sense for there to be a default constructor as well, since there are sensible defaults.

Consider as well whether you actually want to delete the copy constructor. You say that you want this to be used from multiple threads, and although they might all access it through a std::shared_ptr or reference, you might legitimately want to pass each thread its own copy. That way, the object only needs to serialize the output operation itself, not all access from any of the threads.

Support Logging to any std::ostream

Currently, you only log to stdout from the C standard library. It would be better to tell the logger which file or stream to output to when it is constructed, defaulting to std::cerr or perhaps std::clog. You can instead construct a logger that logs to a file, by passing it a std::ofstream&.

Use C++ Style I/O, or std::format

Currently, the interface is a C-style format string and variadic arguments, but without the checks the compiler does for correctness. This is obsolete and finicky.

If you accept a std::string_view, you can pass in any string-like object, including the return value of std::format. You probably want an additional overload for const char*, to cover all your bases. It also guarantees that each log entry is a single output call, which (as G. Sliepen reminded me), is already thread-safe.

Alternatively, you can overload operator<< to accept any type that is overloaded to output to an ostream&.

Use std::source_location Instead of C-style Macros

A better solution than a C-style macro for passing the logger the source line that calls it is std::source_location. Give the logging function a default final argument const std::source_location srcl = std::source_location::current(). Then, the compiler will default to passing the function the source location of the call site.

Also Replace the Other C-Style Macros

In general, use macros only when you have to. A good solution here would be to replace the macros that do formatting with wrappers that return a std::string object from std::format or std::stringstream, and pass (or maybe move) it to the logger’s output function in a tail-call. The string will automatically be destroyed when the logger is done with it and returns.

Use the Mutex

Having the threads claim a mutex to be able to log to the stream is a great idea, but threads need to actually lock the mutex before doing I/O.

If you are writing to a stream that something other than the log object might write to, it could be worthwhile to pass the constructor a reference to the mutex it should be using. That way, other functions can also lock the same mutex and interoperate with the logger. By default, the logger can still create its own mutex, which only it uses.

Are You Sure About the Private and Deleted Constructors?

There doesn’t appear to be a way to actually create one of these. If it’s meant to be a base class, the private: constructors should be protected:, so daughter classes can implicitly call them, and also should have a protected:, virtual destructor. If this is meant to be instantiated, it needs at least one public: constructor. It probably makes sense for there to be a default constructor as well, since there are sensible defaults.

Consider as well whether you actually want to delete the copy constructor. You say that you want this to be used from multiple threads, and although they might all access it through a std::shared_ptr or reference, you might legitimately want to pass each thread its own copy. That way, the object only needs to serialize the output operation itself, not all access from any of the threads.

added 124 characters in body
Source Link
Davislor
  • 8.1k
  • 17
  • 36

Support Logging to any std::ostream

Currently, you only log to stdout from the C standard library. It would be better to tell the logger which file or stream to output to when it is constructed, defaulting to std::cerr or perhaps std::clog. You can instead construct a logger that logs to a file, by passing it a std::ofstream&.

Use C++ Style I/O, or std::format

Currently, the interface is a C-style format string and variadic arguments, but without the checks the compiler does for correctness. This is obsolete and finicky.

If you accept a std::string_view, you can pass in any string-like object, including the return value of std::format. You probably want an additional overload for const char*, to cover all your bases. It also guarantees that each log entry is a single output call, which (as G. Sliepen reminded me), is already thread-safe.

Alternatively, if your target is a ofstream, you can overload operator<< to accept any type that is overloaded to output to an ostream&.

Use std::source_location Instead of C-style Macros

A better solution than a C-style macro for passing the logger the source line that calls it is std::source_location. Give the logging function a default final argument const std::source_location srcl = std::source_location::current(). Then, the compiler will default to passing the function the source location of the call site.

Also Replace the Other C-Style Macros

In general, use macros only when you have to. A good solution here would be to replace the macros that do formatting with wrappers that return a std::string object from std::format or std::stringstream, and pass it to the logger’s output function in a tail-call. The string will automatically be destroyed when the logger is done with it and returns.

Use the Mutex

Having the threads claim a mutex to be able to log to the stream is a great idea, but threads need to actually lock the mutex before doing I/O.

If you are writing to a stream that something other than the log object might write to, it could be worthwhile to pass the constructor a reference to the mutex it should be using. That way, other functions can also lock the same mutex and interoperate with the logger. By default, the logger can still create its own mutex, which only it uses.

Are You Sure About the Private and Deleted Constructors?

There doesn’t appear to be a way to actually create one of these. If it’s meant to be a base class, the private: constructors should be protected:, so daughter classes can implicitly call them, and also should have a protected:, virtual destructor. If this is meant to be instantiated, it needs at least one public: constructor. It probably makes sense for there to be a default constructor as well, since there are sensible defaults.

Consider as well whether you actually want to delete the copy constructor. You say that you want this to be used from multiple threads, and although they might all access it through a std::shared_ptr or reference, you might legitimately want to pass each thread its own copy. That way, the object only needs to serialize the output operation itself, not all access from any of the threads.

Support Logging to any std::ostream

Currently, you only log to stdout from the C standard library. It would be better to tell the logger which file or stream to output to when it is constructed, defaulting to std::cerr or perhaps std::clog. You can instead construct a logger that logs to a file, by passing it a std::ofstream&.

Use C++ Style I/O, or std::format

Currently, the interface is a C-style format string and variadic arguments, but without the checks the compiler does for correctness. This is obsolete and finicky.

If you accept a std::string_view, you can pass in any string-like object, including the return value of std::format. You probably want an additional overload for const char*, to cover all your bases.

Alternatively, if your target is a ofstream, you can overload operator<< to accept any type that is overloaded to output to an ostream&.

Use std::source_location Instead of C-style Macros

A better solution than a C-style macro for passing the logger the source line that calls it is std::source_location. Give the logging function a default final argument const std::source_location srcl = std::source_location::current(). Then, the compiler will default to passing the function the source location of the call site.

Also Replace the Other C-Style Macros

In general, use macros only when you have to. A good solution here would be to replace the macros that do formatting with wrappers that return a std::string object from std::format or std::stringstream, and pass it to the logger’s output function in a tail-call. The string will automatically be destroyed when the logger is done with it and returns.

Use the Mutex

Having the threads claim a mutex to be able to log to the stream is a great idea, but threads need to actually lock the mutex before doing I/O.

If you are writing to a stream that something other than the log object might write to, it could be worthwhile to pass the constructor a reference to the mutex it should be using. That way, other functions can also lock the same mutex and interoperate with the logger. By default, the logger can still create its own mutex, which only it uses.

Are You Sure About the Private and Deleted Constructors?

There doesn’t appear to be a way to actually create one of these. If it’s meant to be a base class, the private: constructors should be protected:, so daughter classes can implicitly call them, and also should have a protected:, virtual destructor. If this is meant to be instantiated, it needs at least one public: constructor. It probably makes sense for there to be a default constructor as well, since there are sensible defaults.

Consider as well whether you actually want to delete the copy constructor. You say that you want this to be used from multiple threads, and although they might all access it through a std::shared_ptr or reference, you might legitimately want to pass each thread its own copy. That way, the object only needs to serialize the output operation itself, not all access from any of the threads.

Support Logging to any std::ostream

Currently, you only log to stdout from the C standard library. It would be better to tell the logger which file or stream to output to when it is constructed, defaulting to std::cerr or perhaps std::clog. You can instead construct a logger that logs to a file, by passing it a std::ofstream&.

Use C++ Style I/O, or std::format

Currently, the interface is a C-style format string and variadic arguments, but without the checks the compiler does for correctness. This is obsolete and finicky.

If you accept a std::string_view, you can pass in any string-like object, including the return value of std::format. You probably want an additional overload for const char*, to cover all your bases. It also guarantees that each log entry is a single output call, which (as G. Sliepen reminded me), is already thread-safe.

Alternatively, you can overload operator<< to accept any type that is overloaded to output to an ostream&.

Use std::source_location Instead of C-style Macros

A better solution than a C-style macro for passing the logger the source line that calls it is std::source_location. Give the logging function a default final argument const std::source_location srcl = std::source_location::current(). Then, the compiler will default to passing the function the source location of the call site.

Also Replace the Other C-Style Macros

In general, use macros only when you have to. A good solution here would be to replace the macros that do formatting with wrappers that return a std::string object from std::format or std::stringstream, and pass it to the logger’s output function in a tail-call. The string will automatically be destroyed when the logger is done with it and returns.

Use the Mutex

Having the threads claim a mutex to be able to log to the stream is a great idea, but threads need to actually lock the mutex before doing I/O.

If you are writing to a stream that something other than the log object might write to, it could be worthwhile to pass the constructor a reference to the mutex it should be using. That way, other functions can also lock the same mutex and interoperate with the logger. By default, the logger can still create its own mutex, which only it uses.

Are You Sure About the Private and Deleted Constructors?

There doesn’t appear to be a way to actually create one of these. If it’s meant to be a base class, the private: constructors should be protected:, so daughter classes can implicitly call them, and also should have a protected:, virtual destructor. If this is meant to be instantiated, it needs at least one public: constructor. It probably makes sense for there to be a default constructor as well, since there are sensible defaults.

Consider as well whether you actually want to delete the copy constructor. You say that you want this to be used from multiple threads, and although they might all access it through a std::shared_ptr or reference, you might legitimately want to pass each thread its own copy. That way, the object only needs to serialize the output operation itself, not all access from any of the threads.

added 899 characters in body
Source Link
Davislor
  • 8.1k
  • 17
  • 36

Support Logging to any std::ostream

Currently, you only log to stdout from the C standard library. It would be better to tell the logger which file or stream to output to when it is constructed, defaulting to std::cerr or perhaps std::clog. You can instead construct a logger that logs to a file, by passing it a std::ofstream&.

Use C++ Style I/O, or std::format

Currently, the interface is a C-style format string and variadic arguments, but without the checks the compiler does for correctness. This is obsolete and finicky.

If you accept a std::string_view, you can pass in any string-like object, including the return value of std::format. You probably want an additional overload for const char*, to cover all your bases.

Alternatively, if your target is a ofstream, you can overload operator<< to accept any type that is overloaded to output to an ostream&.

Use std::source_location Instead of C-style Macros

A better solution than a C-style macro for passing the logger the source line that calls it is std::source_location. Give the logging function a default final argument const std::source_location srcl = std::source_location::current(). Then, the compiler will default to passing the function the source location of the call site.

Also Replace the Other C-Style Macros

In general, use macros only when you have to. A good solution here would be to replace the macros that do formatting with wrappers that return a std::string object from std::format or std::stringstream, and pass it to the logger’s output function in a tail-call. The string will automatically be destroyed when the logger is done with it and returns.

Use the Mutex

Having the threads claim a mutex to be able to log to the stream is a great idea, but threads need to actually lock the mutex before doing I/O.

If you are writing to a stream that something other than the log object might write to, it could be worthwhile to pass the constructor a reference to the mutex it should be using. That way, other functions can also lock the same mutex and interoperate with the logger. By default, the logger can still create its own mutex, which only it uses.

Are You Sure About the Private and Deleted Constructors?

There doesn’t appear to be a way to actually create one of these. If it’s meant to be a base class, the private: constructors should be protected:, so daughter classes can implicitly call them, and also should have a protected:, virtual destructor. If this is meant to be instantiated, it needs at least one public: constructor. It probably makes sense for there to be a default constructor as well, since there are sensible defaults.

Consider as well whether you actually want to delete the copy constructor. You say that you want this to be used from multiple threads, and although they might all access it through a std::shared_ptr or reference, you might legitimately want to pass each thread its own copy. That way, the object only needs to serialize the output operation itself, not all access from any of the threads.

Support Logging to any std::ostream

Currently, you only log to stdout from the C standard library. It would be better to tell the logger which file or stream to output to when it is constructed, defaulting to std::cerr or perhaps std::clog.

Use C++ Style I/O, or std::format

Currently, the interface is a C-style format string and variadic arguments, but without the checks the compiler does for correctness. This is obsolete and finicky.

If you accept a std::string_view, you can pass in any string-like object, including the return value of std::format. You probably want an additional overload for const char*, to cover all your bases.

Alternatively, if your target is a ofstream, you can overload operator<< to accept any type that is overloaded to output to an ostream&.

Use std::source_location Instead of C-style Macros

A better solution than a C-style macro for passing the logger the source line that calls it is std::source_location. Give the logging function a default final argument const std::source_location srcl = std::source_location::current(). Then, the compiler will default to passing the function the source location of the call site.

Also Replace the Other C-Style Macros

In general, use macros only when you have to. A good solution here would be to replace the macros that do formatting with wrappers that return a std::string object from std::format or std::stringstream, and pass it to the logger’s output function in a tail-call. The string will automatically be destroyed when the logger is done with it and returns.

Use the Mutex

Having the threads claim a mutex to be able to log to the stream is a great idea, but threads need to actually lock the mutex before doing I/O.

If you are writing to a stream that something other than the log object might write to, it could be worthwhile to pass the constructor a reference to the mutex it should be using. That way, other functions can also lock the same mutex and interoperate with the logger. By default, the logger can still create its own mutex, which only it uses.

Support Logging to any std::ostream

Currently, you only log to stdout from the C standard library. It would be better to tell the logger which file or stream to output to when it is constructed, defaulting to std::cerr or perhaps std::clog. You can instead construct a logger that logs to a file, by passing it a std::ofstream&.

Use C++ Style I/O, or std::format

Currently, the interface is a C-style format string and variadic arguments, but without the checks the compiler does for correctness. This is obsolete and finicky.

If you accept a std::string_view, you can pass in any string-like object, including the return value of std::format. You probably want an additional overload for const char*, to cover all your bases.

Alternatively, if your target is a ofstream, you can overload operator<< to accept any type that is overloaded to output to an ostream&.

Use std::source_location Instead of C-style Macros

A better solution than a C-style macro for passing the logger the source line that calls it is std::source_location. Give the logging function a default final argument const std::source_location srcl = std::source_location::current(). Then, the compiler will default to passing the function the source location of the call site.

Also Replace the Other C-Style Macros

In general, use macros only when you have to. A good solution here would be to replace the macros that do formatting with wrappers that return a std::string object from std::format or std::stringstream, and pass it to the logger’s output function in a tail-call. The string will automatically be destroyed when the logger is done with it and returns.

Use the Mutex

Having the threads claim a mutex to be able to log to the stream is a great idea, but threads need to actually lock the mutex before doing I/O.

If you are writing to a stream that something other than the log object might write to, it could be worthwhile to pass the constructor a reference to the mutex it should be using. That way, other functions can also lock the same mutex and interoperate with the logger. By default, the logger can still create its own mutex, which only it uses.

Are You Sure About the Private and Deleted Constructors?

There doesn’t appear to be a way to actually create one of these. If it’s meant to be a base class, the private: constructors should be protected:, so daughter classes can implicitly call them, and also should have a protected:, virtual destructor. If this is meant to be instantiated, it needs at least one public: constructor. It probably makes sense for there to be a default constructor as well, since there are sensible defaults.

Consider as well whether you actually want to delete the copy constructor. You say that you want this to be used from multiple threads, and although they might all access it through a std::shared_ptr or reference, you might legitimately want to pass each thread its own copy. That way, the object only needs to serialize the output operation itself, not all access from any of the threads.

Source Link
Davislor
  • 8.1k
  • 17
  • 36
Loading