[Lldb-commits] [PATCH] D29514: Change Error::PutToLog to LLDB_LOG_ERROR

[Zachary Turner via Phabricator via lldb-commits]

zturner added a comment.

In https://reviews.llvm.org/D29514#666285, @labath wrote:

> I'm not opposed to this patch, if people really want it, but I don't really see the value of this macro.
>  Why couldn't I write
>  `LLDB_LOG(log, "foo: {0}", error);`
>  instead of
>  `LLDB_LOG_ERROR(log, error, "foo");`
>  Am I missing something?


I'm all for making it simpler.  The reason I did it this way is that I wanted to keep the exact same syntax as before to minimize the impact of the change.  It's rather confusing, but suppose you have these two errors:

1. E1 = Success, message = "The operation completed successfully", code = 0
2. E2 = Error, message = "The operation failed", code = 0x12345

When you call `PutToLog(E1, "While reading the %d'th item", 7)` it will log the string `While reading the 7'th item err = 0x0`
When you call `PutToLog(E2, "While reading the %d'th item", 7)` it will log the string `error: While reading the 7'th item err = The operation failed (0x12345)`

If we do what you suggest, then we are relying on the `lldb::Error` format provider, which does not have the ability to accept this kind of "contextual reason" string (i.e. the string "While reading the %d'th item" in the above example).  So if we did this:

  LLDB_LOG(log, error, "While reading the {0}'th item", 7)

Then the entire error must be formatted independently of the entire context string.  So we could produce the output `error: The operation failed (0x12345), context: While reading the 7'th item` but this is slightly different from what was being printed before.

I didn't actually try this approach and see if anything broke, because Windows runs much fewer tests than the rest of LLDB, so even if my test suite passed, it wouldn't guarantee that another test somewhere else wasn't relying on the message being exactly as it currently prints.


https://reviews.llvm.org/D29514