Any way to avoid using a macro for this C++ scoped logger?

Question

I am refreshing some very old code in our diagnostic library to modern C++, and one piece of code remains that has always bothered me. It is a very common kind of scoped logging function that uses a macro to define the storage of logging data in an application-specific slot, simplified here as an int, with an optional version that takes a condition which is used to decide whether the timing should happen (for specific cases like first time through a loop, or when under extreme artificial external timing pressure, for example).

class ScopedTimer
{
   int mSlot;
   bool mCondition;
   Ticker mBeginTimer;
public:
  ScopedTimer( int slot, bool condition ) : 
      mSlot( slot ), 
      mCondition(condition)
   {
       if ( mCondition ) mBeginTimer = GetTimer(); 
   }

  ~ScopedTimer() 
  { 
       if (mCondition)
       {
           StoreInSlot( mSlot, GetTimer() - mBeginTimer ); 
       }
  }
};

#ifdef PROFILING_ENABLED
#define ST_NAME_CONCAT( x, y )      x ## _ ## y
#define ST_NAME_EVALUATOR( x, y ) ST_NAME_CONCAT( x, y )
#define ST_NAME( prefix ) ST_NAME_EVALUATOR( prefix, __LINE__ )
#define SCOPED_TIMER(slot) ScopedTimer ST_NAME(scopedTimer)( slot, condition )
#else
#define SCOPED_TIMER(slot) // goes away in release
#endif

and in use:

extern bool ConditionRequestsTiming();

void foo()
{
  SCOPED_TIMER( some_magic_slot_number, ConditionRequestsTiming() );

  ... other operations

}

Expands to

void foo()
{
  ScopedTimer scopedTimer12( some_magic_slot_number, ConditionRequestsTiming() );

  ... other operations

  // calling ~ScopedTimer() here, but only if ConditionRequestsTiming was true, storing time difference in the slot
}

And in release:

void foo()
{
  ... other operations
}

Which of course works as expected, giving a scoped timer for the method. It is very convenient, and less error prone, but it doesn't respect namespaces and that's always bothered me.

Any thoughts on a modern solution without too much complication, or should I just accept this pattern as okay for our diagnostic library?

Your macro doesn't use any pre-defined macros (`__LINE__`, etc) so what's wrong with `ScopedTimer a_name_that_is_unique_in_the_scope(some_magic_slot_number)`? Also, any identifier with a double underscore is reserved to the implementation in C++, and your macro uses `__scopedTimer` so has undefined behaviour whenever it is used. — Peter, Apr 05 '21 at 06:39
When doing this, always remember that the destructor may not throw. Else you'll have a problem. So I hope your "storing time difference in the slot" is safe. Anyhow, IMHO adding a macro which only function is to expand "SCOPED_TIMER" to "ScopedTimer __scopedTimer" is adding unnecessary complexity and making it more error prone. Finally: you shouldn't use double underscore identifiers, as they're used for the compiler internal structures.... — JHBonarius, Apr 05 '21 at 06:40
The macro definition should probably not include the semicolon; that will be provided after the macro is invoked. As it is, you'd end up with an extra null statement, which can be problematic and is (probably) not what's intended. — Jonathan Leffler, Apr 05 '21 at 14:42
@JHBonarius the key for us is removing the code from release builds since it is part of the instrumentation library. — Steven, Apr 05 '21 at 15:53

score 6 · Answer 1 · answered Apr 05 '21 at 06:18

6

Usually people use macros to append __LINE__ to the declaration to allow for multiple declarations in one scope block.

Prior to C++20 this was impossible without macros. C++20 and later, with a little work, can use std::source_location.

Jason Turner has a video on it in his C++ weekly video series here

answered Apr 05 '21 at 06:18

Casey

10,297
11
59
88

Casey, I forgot to include that code in our sample. I will look at std::source_location – Steven Apr 05 '21 at 15:53

MatG · Answer 2 · 2021-04-05T07:15:05.477

2

I was initially not sure to grasp the advantage of that macro, apart hiding the timer instance name (and cause possible conflicts). But I think that the intent could be to have the possibility to do this:

#ifdef _DEBUG
  #define SCOPED_TIMER(slot) ScopedTimer __scopedTimer( slot );
#else
  #define SCOPED_TIMER(slot) ;
#endif

That would indeed save some keystrokes; otherwise, if the timing takes place also in release builds, I would simply use directly the macro definition; either way, I would get rid of the initial underscores in the object name (that are conventionally reserved for compiler implementers):

ScopedTimer scoped_timer( some_magic_slot_number );

edited Apr 05 '21 at 07:15

answered Apr 05 '21 at 06:51

MatG

574
2
7
19

2

The macro definitions should probably not include the semicolon; that will be provided after the macro is invoked. As it is, you'd end up with a null statement (or two null statements) in a row, which can be problematic and is (probably) not what's intended. – Jonathan Leffler Apr 05 '21 at 14:32
1

This is exactly why we use a macro. – Steven Apr 05 '21 at 15:53
@JonathanLeffler I initially misunderstood and instantly compiled multiple semicolons with gcc, clang and MSVC checking for any `NOP` or changes in the binary output! Well, I agree with you, I just followed the OP original (first version) `#define` with the semicolon... – MatG Apr 05 '21 at 19:12

Any way to avoid using a macro for this C++ scoped logger?

2 Answers2