Skip to content

Commit d1223eb

Browse files
refactor(logging): capture Builder source_location at the caller's site
Take std::source_location as a defaulted constructor argument so Builder(level) records the caller's file/line by default; current() in the init-list would record logger.h instead. Add tests for the default caller-site location and the Location() override. Co-authored-by: Isaac
1 parent 2cb7357 commit d1223eb

2 files changed

Lines changed: 13 additions & 13 deletions

File tree

src/iceberg/logging/logger.h

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -81,11 +81,14 @@ struct ICEBERG_EXPORT LogMessage {
8181
/// .Build();
8282
/// logger->Log(std::move(record));
8383
///
84-
/// The location defaults to where the Builder is constructed; override it with
85-
/// Location() (e.g. to forward a caller's std::source_location).
84+
/// The location defaults to the caller's construction site (captured via the
85+
/// constructor's default argument); override it with Location() (e.g. to forward
86+
/// a caller's std::source_location).
8687
class ICEBERG_EXPORT LogMessage::Builder {
8788
public:
88-
explicit Builder(LogLevel level) : level_(level), location_(std::source_location::current()) {}
89+
explicit Builder(LogLevel level,
90+
std::source_location location = std::source_location::current())
91+
: level_(level), location_(location) {}
8992

9093
/// \brief Set the already-formatted message text.
9194
Builder& Message(std::string message) {

src/iceberg/test/logger_test.cc

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@
2121

2222
#include <atomic>
2323
#include <memory>
24-
#include <string_view>
2524
#include <thread>
2625
#include <tuple>
2726
#include <vector>
@@ -430,16 +429,14 @@ TEST(LoggerTest, BuilderDefaultsAndEmitToSink) {
430429
EXPECT_NE(records[0].location.line(), 0u); // location defaulted at build site
431430
}
432431

433-
// location_ is initialized in the Builder constructor, so when Location() is not
434-
// called the default points at the constructor itself (in logger.h), not at the
435-
// caller and not at line 0.
436-
TEST(LoggerTest, BuilderDefaultLocationIsConstructorSite) {
432+
// The constructor captures source_location as a default argument, so without
433+
// Location() the default is the caller's construction site (this file), not
434+
// logger.h. The Builder is constructed exactly one line below `here`.
435+
TEST(LoggerTest, BuilderDefaultLocationIsCallerSite) {
436+
auto here = std::source_location::current();
437437
auto record = LogMessage::Builder(LogLevel::kInfo).Message("m").Build();
438-
EXPECT_NE(record.location.line(), 0u);
439-
EXPECT_NE(std::string_view(record.location.file_name()).find("logger.h"),
440-
std::string_view::npos);
441-
EXPECT_NE(std::string_view(record.location.function_name()).find("Builder"),
442-
std::string_view::npos);
438+
EXPECT_STREQ(record.location.file_name(), here.file_name());
439+
EXPECT_EQ(record.location.line(), here.line() + 1);
443440
}
444441

445442
// Location() replaces the constructor default with the caller's site (file + line).

0 commit comments

Comments
 (0)