- 
          
 - 
                Notifications
    
You must be signed in to change notification settings  - Fork 3.2k
 
Add Unscoped Capture Logger #3010
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: devel
Are you sure you want to change the base?
Conversation
b6d1042    to
    d34e490      
    Compare
  
    
          Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@            Coverage Diff             @@
##            devel    #3010      +/-   ##
==========================================
+ Coverage   90.92%   90.95%   +0.03%     
==========================================
  Files         201      201              
  Lines        8653     8659       +6     
==========================================
+ Hits         7867     7875       +8     
+ Misses        786      784       -2     🚀 New features to boost your workflow:
  | 
    
4e18ef2    to
    98c9b6f      
    Compare
  
    c3d0075    to
    abfdfcf      
    Compare
  
            
          
                src/catch2/catch_message.hpp
              
                Outdated
          
        
      | std::vector<MessageInfo> getMessageDetails() const { | ||
| return m_messages; | ||
| } | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not used anymore. For next time, this shouldn't be returning the vector by value, but by reference, and be ref-qualified if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed it
        
          
                src/catch2/catch_message.hpp
              
                Outdated
          
        
      | void captureUnscopedValue( size_t index, std::string const& value ); | ||
| 
               | 
          ||
| template<typename T> | ||
| void captureUnscopedValues( size_t index, T const& value ) { | ||
| captureUnscopedValue( index, Catch::Detail::stringify( value ) ); | ||
| } | ||
| 
               | 
          ||
| template<typename T, typename... Ts> | ||
| void captureUnscopedValues( size_t index, T const& value, Ts const&... values ) { | ||
| captureUnscopedValue( index, Catch::Detail::stringify(value) ); | ||
| captureUnscopedValues( index+1, values... ); | ||
| } | ||
| 
               | 
          
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not duplicate these, and instead add bool isScoped parameter to the constructor of Capturer, which will be kept to decide whether captureValue should create scoped or unscoped msg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah modified this
        
          
                src/catch2/catch_message.cpp
              
                Outdated
          
        
      | getResultCapture().emplaceUnscopedMessage(Catch::MessageBuilder( | ||
| m_messages[index].macroName, | ||
| m_messages[index].lineInfo, | ||
| m_messages[index].type) << m_messages[index].message); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to copy the value into message first and then write it into the msg builder stream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
        
          
                src/catch2/catch_message.hpp
              
                Outdated
          
        
      | #define UNSCOPED_INFO( msg ) (void)(0) | ||
| #define WARN( msg ) (void)(0) | ||
| #define CAPTURE( ... ) (void)(0) | ||
| #define UNSCOPED_CAPTURE( ... ) (void)(0) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please re-align the empty definitions for the whole block. (And same for the one around line 150)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
466e345    to
    d1ede1a      
    Compare
  
    d1ede1a    to
    7c551dd      
    Compare
  
    | 
           @horenmar feel free to take a look  | 
    
7c551dd    to
    9f4c665      
    Compare
  
    9f4c665    to
    5abeb26      
    Compare
  
    | std::vector<MessageInfo> m_messages; | ||
| IResultCapture& m_resultCapture; | ||
| size_t m_captured = 0; | ||
| bool isScoped; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take a second look at how the other class variables are named.
| m_resultCapture.pushScopedMessage( m_messages[index] ); | ||
| m_captured++; | ||
| if(isScoped){ | ||
| assert( index < m_messages.size() ); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assert should live at the start of the function, both branches access m_messages[index].
| if(index == m_messages.size() - 1){ | ||
| m_messages.clear(); | ||
| } | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to clear this here, instead check in destructor whether m_isScoped is true and change it accordingly.
Adding Unscoped Capture Logger as a part of #2954
Description
Added Unscoped Capture Logger Feature as required to print details related to Capture
GitHub Issues
Closes #2954