[Lldb-commits] [PATCH] D65128: [Logging] Replace Log::Printf with LLDB_LOG macro (NFC)

Jim Ingham via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Thu Jul 25 10:51:35 PDT 2019

jingham added inline comments.

Comment at: lldb/source/Expression/IRExecutionUnit.cpp:601
   if (log) {
+    LLDB_LOGF(log,
labath wrote:
> jingham wrote:
> > labath wrote:
> > > looks like there are some `if(log)`s still remaining. Maybe the `{}` around the printf confused your vim macro?
> > There will always be some "if (log)" statements.  Whenever consing up the log message involves work, you should always say:
> > 
> > if (log) {
> >    /// Do expensive work
> >    LLDB_LOG...
> > }
> > 
> > Not saying that all the remaining "if (logs)" are for that reason, but then intent is NOT to remove all such statements as they serve a useful purpose.
> I'm fine with `if(log)`s staying if they serve a purpose (or even if they're hard to remove in this patch). However, I would expect that with LLDB_LOG, they become much less frequently needed, because the formatter is much more powerful (e.g., there is no need to write a for loop just to print an array, etc.)
Yes, it's also not entirely obvious (YAY for macros!!!) that code in the LLDB_LOG* arguments doesn't get evaluated if log == nullptr, but that also reduces the need for standalone if (logs).

BTW, we were going to document somewhere that LLDB_LOG actually used format_providers but I didn't see that anywhere.  I submitted:


to put this somewhere in the lldb sources.  Not sure if this is the best place, or if there is more that it would be useful to add.




More information about the lldb-commits mailing list