Implement Jonas's review suggestions.

Author: jimingham

lldb/include/lldb/Breakpoint/BreakpointLocation.h

@@ -409,38 +409,39 @@ class BreakpointLocation
   bool m_should_resolve_indirect_functions;
   bool m_is_reexported;
   bool m_is_indirect;
-  Address m_address;   ///< The address defining this location.
-  Breakpoint &m_owner; ///< The breakpoint that produced this object.
-  std::unique_ptr<BreakpointOptions> m_options_up; ///< Breakpoint options
-                                                   /// pointer, nullptr if we're
-                                                   /// using our breakpoint's
-                                                   /// options.
-  lldb::BreakpointSiteSP m_bp_site_sp; ///< Our breakpoint site (it may be
-                                       ///shared by more than one location.)
-  lldb::UserExpressionSP m_user_expression_sp; ///< The compiled expression to
-                                               ///use in testing our condition.
-  std::mutex m_condition_mutex; ///< Guards parsing and evaluation of the
-                                ///condition, which could be evaluated by
-                                /// multiple processes.
-  size_t m_condition_hash; ///< For testing whether the condition source code
-                           ///changed.
-  lldb::break_id_t m_loc_id; ///< Breakpoint location ID.
-  StoppointHitCounter m_hit_counter; ///< Number of times this breakpoint
-                                     /// location has been hit.
+  ///< The address defining this location.
+  Address m_address;
+  ///< The breakpoint that produced this object.
+  Breakpoint &m_owner;
+  ///< Breakpoint options pointer, nullptr if we're using our breakpoint's
+  /// options.
+  std::unique_ptr<BreakpointOptions> m_options_up;
+  ///< Our breakpoint site (it may be shared by more than one location.) 
+  lldb::BreakpointSiteSP m_bp_site_sp;
+  ///< The compiled expression to use in testing our condition.
+  lldb::UserExpressionSP m_user_expression_sp;
+  ///< Guards parsing and evaluation of the condition, which could be evaluated
+  /// by multiple processes. 
+  std::mutex m_condition_mutex;
+  ///< For testing whether the condition source code changed. 
+  size_t m_condition_hash;
+  ///< Breakpoint location ID.
+  lldb::break_id_t m_loc_id;
+  ///< Number of times this breakpoint location has been hit.
+  StoppointHitCounter m_hit_counter; 
   /// If this exists, use it to print the stop description rather than the
   /// LineEntry m_address resolves to directly.  Use this for instance when the
   /// location was given somewhere in the virtual inlined call stack since the
   /// Address always resolves to the lowest entry in the stack.
   std::optional<LineEntry> m_preferred_line_entry;
-  bool m_is_valid = true;   /// Because Facade locations don't have sites
-                            /// we can't use the presence of the site to mean
-                            /// this breakpoint is valid, but must manage
-                            /// the state directly.
-  bool m_is_facade = false; /// Facade locations aren't directly triggered
-                            /// and don't have a breakpoint site.  They are
-                            /// a useful fiction when you want to represent
-                            /// the stop location as something lldb can't
-                            /// naturally stop at.
+  /// Because Facade locations don't have sites we can't use the presence of 
+  /// the site to mean this breakpoint is valid, but must manage the state 
+  /// directly.
+  bool m_is_valid = true;
+  /// Facade locations aren't directly triggered and don't have a breakpoint
+  /// site.  They are a useful fiction when you want to represent the stop 
+  /// location as something lldb can't naturally stop at. 
+  bool m_is_facade = false;
 
   void SetInvalid() { m_is_valid = false; }
 

lldb/source/API/SBBreakpoint.cpp

@@ -569,9 +569,9 @@ SBError SBBreakpoint::AddLocation(SBAddress &address) {
 
 SBBreakpointLocation SBBreakpoint::AddFacadeLocation() {
   BreakpointSP bkpt_sp = GetSP();
-  if (!bkpt_sp) {
+  if (!bkpt_sp)
     return {};
-  }
+
   BreakpointLocationSP loc_sp = bkpt_sp->AddFacadeLocation();
   return SBBreakpointLocation(loc_sp);
 }

lldb/source/Breakpoint/BreakpointLocation.cpp

@@ -529,8 +529,7 @@ llvm::Error BreakpointLocation::ClearBreakpointSite() {
     // This might be a Facade Location, which don't have sites or addresses
     if (IsFacade())
       return llvm::Error::success();
-    else
-      return llvm::createStringError("no breakpoint site to clear");
+    return llvm::createStringError("no breakpoint site to clear");
   }
 
   // If the process exists, get it to remove the owner, it will remove the
@@ -692,7 +691,7 @@ void BreakpointLocation::GetDescription(Stream *s,
     }
   }
 
-  // Scripted breakpoint are currently always resolved.  Does this seem right?
+  // FIXME: scripted breakpoint are currently always resolved.  Does this seem right?
   // If they don't add any scripted locations, we shouldn't consider them
   // resolved.
   bool is_resolved = is_scripted_desc || IsResolved();

lldb/source/Breakpoint/BreakpointResolverScripted.cpp

@@ -173,7 +173,6 @@ BreakpointResolverScripted::WasHit(lldb::StackFrameSP frame_sp,
                                    lldb::BreakpointLocationSP bp_loc_sp) {
   if (m_interface_sp)
     return m_interface_sp->WasHit(frame_sp, bp_loc_sp);
-
   return {};
 }
 

lldb/source/Interpreter/ScriptInterpreter.cpp

@@ -108,9 +108,8 @@ ScriptInterpreter::GetStatusFromSBError(const lldb::SBError &error) const {
 
 lldb::StackFrameSP
 ScriptInterpreter::GetOpaqueTypeFromSBFrame(const lldb::SBFrame &frame) const {
-  if (frame.m_opaque_sp) {
+  if (frame.m_opaque_sp)
     return frame.m_opaque_sp->GetFrameSP();
-  }
   return nullptr;
 }