Repository navigation
Commit f7b5b9b
Tyler Hovanec
Replace global LogError/LogWarning/LogInfo/LogDebug macros with inline functions
Motivation:
`cpp/open3d/utility/Logging.h` previously defined `LogError`, `LogWarning`,
`LogInfo`, and `LogDebug` as unprefixed preprocessor macros in the global
preprocessor scope (`#define LogError(...) Logger::LogError_(...)`) to mimic
a "macro in namespace" when callers write `utility::LogError(...)`.
Because C++ preprocessor macros do not respect namespaces, including any Open3D
header that transitively includes `open3d/utility/Logging.h` (such as
`open3d/geometry/TriangleMesh.h` or `open3d/geometry/PointCloud.h`) pollutes
the global macro namespace for the remainder of the translation unit. Any
subsequently included third-party or application header that declares a method,
function, or enum named `LogError`, `LogWarning`, `LogInfo`, or `LogDebug`
(for example, custom logger interfaces or Abseil logging utilities) fails to
compile because the preprocessor rewrites `foo.LogError(...)` into
`foo.Logger::LogError_(__FILE__, __LINE__, ...)`.
Why This Is Possible Now (Without Requiring C++20):
When `Logging.h` was originally written in 2019 (targeting C++14 and older
compilers), preprocessor macros (`__FILE__`, `__LINE__`, `OPEN3D_FUNCTION`)
were the only portable way to capture caller source locations because Clang
(< 9.0, Sept 2019) and MSVC (< VS 2019 16.6 / `_MSC_VER 1926`, May 2020) did
not yet implement the compiler intrinsics (`__builtin_FILE()`,
`__builtin_LINE()`, `__builtin_FUNCTION()`) created to back C++20
`std::source_location`. Even though the `<source_location>` header itself
requires C++20, all toolchains supported by Open3D today (GCC 7+, Clang 9+,
AppleClang, MSVC VS 2019 16.6+, and NVCC) expose `__builtin_FILE()`,
`__builtin_LINE()`, and `__builtin_FUNCTION()` in C++17 mode.
Why Functionality Is Equivalent:
1. Call-site syntax is unchanged: `utility::LogError(...)`,
`open3d::utility::LogError(...)`, and unqualified `LogError(...)` (inside
`namespace open3d::utility` or translation units with
`using namespace open3d::utility;`) all resolve to the new `inline` function
templates in `namespace open3d::utility`.
2. Caller source location (`file`, `line`, `function`) is still captured at the
call site without C++20 `<source_location>` or macros:
`open3d::utility::detail::LogLocation` wraps `const char *format` with a
non-explicit `constexpr` constructor whose default arguments
(`__builtin_FILE()`, `__builtin_LINE()`, `__builtin_FUNCTION()`) are
evaluated at the caller's source location in C++17 across GCC, Clang,
AppleClang, MSVC (VS 2019 16.6+), and NVCC.
3. Formatting and control-flow semantics are untouched: `LogError`,
`LogWarning`, `LogInfo`, and `LogDebug` forward `loc.file`, `loc.line`,
`loc.function`, `loc.format`, and `std::forward<Args>(args)...` directly to
`Logger::LogError_`, `Logger::LogWarning_`, `Logger::LogInfo_`, and
`Logger::LogDebug_`, preserving `[[noreturn]]` on `LogError` and existing
`fmt` formatting behavior.
Testing:
- Extended `cpp/tests/utility/Logging.cpp` to verify that `utility::LogError`
captures the caller's filename (`Logging.cpp`) and formatted message in the
thrown `std::runtime_error`.
- Added `Logging.NoGlobalMacroPollution` in `cpp/tests/utility/Logging.cpp` to
verify that a class defining `LogError()`, `LogWarning()`, `LogInfo()`, and
`LogDebug()` member functions compiles and executes cleanly after including
`open3d/utility/Logging.h`.1 parent b6c5e19 commit f7b5b9b
3 files changed
Lines changed: 123 additions & 58 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
99 | 99 | | |
100 | 100 | | |
101 | 101 | | |
| 102 | + | |
102 | 103 | | |
103 | 104 | | |
104 | 105 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
10 | 10 | | |
11 | 11 | | |
12 | 12 | | |
| 13 | + | |
13 | 14 | | |
14 | 15 | | |
15 | 16 | | |
| |||
29 | 30 | | |
30 | 31 | | |
31 | 32 | | |
32 | | - | |
33 | | - | |
34 | | - | |
35 | | - | |
36 | | - | |
37 | | - | |
38 | | - | |
39 | | - | |
40 | | - | |
41 | | - | |
42 | | - | |
43 | | - | |
44 | | - | |
45 | | - | |
46 | | - | |
47 | | - | |
48 | | - | |
49 | | - | |
50 | | - | |
51 | | - | |
52 | | - | |
53 | | - | |
54 | | - | |
55 | | - | |
56 | | - | |
57 | | - | |
58 | | - | |
59 | | - | |
60 | | - | |
61 | | - | |
62 | | - | |
63 | | - | |
64 | | - | |
65 | | - | |
66 | | - | |
67 | | - | |
68 | | - | |
69 | | - | |
70 | | - | |
71 | | - | |
72 | | - | |
73 | | - | |
74 | | - | |
75 | | - | |
76 | | - | |
77 | | - | |
78 | | - | |
79 | | - | |
80 | | - | |
81 | | - | |
82 | | - | |
83 | | - | |
84 | | - | |
85 | | - | |
86 | 33 | | |
87 | 34 | | |
88 | 35 | | |
89 | 36 | | |
90 | | - | |
| 37 | + | |
91 | 38 | | |
92 | 39 | | |
93 | 40 | | |
94 | 41 | | |
95 | 42 | | |
96 | 43 | | |
97 | | - | |
| 44 | + | |
98 | 45 | | |
99 | 46 | | |
100 | 47 | | |
101 | | - | |
102 | | - | |
| 48 | + | |
| 49 | + | |
103 | 50 | | |
104 | 51 | | |
105 | 52 | | |
| |||
235 | 182 | | |
236 | 183 | | |
237 | 184 | | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
| 210 | + | |
| 211 | + | |
| 212 | + | |
| 213 | + | |
| 214 | + | |
| 215 | + | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
| 231 | + | |
| 232 | + | |
| 233 | + | |
| 234 | + | |
| 235 | + | |
| 236 | + | |
| 237 | + | |
| 238 | + | |
| 239 | + | |
| 240 | + | |
| 241 | + | |
| 242 | + | |
| 243 | + | |
| 244 | + | |
| 245 | + | |
| 246 | + | |
| 247 | + | |
| 248 | + | |
| 249 | + | |
| 250 | + | |
| 251 | + | |
| 252 | + | |
| 253 | + | |
| 254 | + | |
| 255 | + | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
| 263 | + | |
238 | 264 | | |
239 | 265 | | |
240 | 266 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
7 | 7 | | |
8 | 8 | | |
9 | 9 | | |
| 10 | + | |
| 11 | + | |
10 | 12 | | |
11 | 13 | | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
12 | 30 | | |
13 | 31 | | |
14 | 32 | | |
15 | 33 | | |
16 | 34 | | |
17 | 35 | | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
18 | 44 | | |
19 | 45 | | |
20 | 46 | | |
21 | 47 | | |
22 | 48 | | |
23 | 49 | | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
24 | 62 | | |
25 | 63 | | |
0 commit comments