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

Jim Ingham via Phabricator via llvm-commits llvm-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:

https://reviews.llvm.org/D65293

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.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65128





More information about the llvm-commits mailing list