[Lldb-commits] [PATCH] D73594: Refactor CommandObjectTargetSymbolsAdd::AddModuleSymbols

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
Comment at: lldb/source/Commands/CommandObjectTarget.cpp:4149
+    
+    lldbassert(matching_modules.GetSize() == 1);
+    ModuleSP module_sp(matching_modules.GetModuleAtIndex(0));
----------------
amccarth wrote:
> clayborg wrote:
> > labath wrote:
> > > This should be a regular assert according to <https://lldb.llvm.org/resources/contributing.html#error-handling-and-use-of-assertions-in-lldb>.
> > This assert will never trigger with the above code already checking for empty on line 4123 and then checking if size > 1 on line 4140. Remove it?
> It's true that this assert won't happen given the early returns above, so I will remove it if you like.
> 
> But I did it intentionally:  (1) to document the requirement and (2) to protect against future changes accidentally breaking the assumptions.  If the function were much shorter, then it might be obvious enough as is.  But early returns, especially halfway down a long function, can be easy to miss.
Gotcha. It is fine to leave it in, just switch to assert() as Pavel mentioned.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73594/new/

https://reviews.llvm.org/D73594