Page MenuHomePhabricator

Remove LIBLLDB_LOG_VERBOSE category
ClosedPublic

Authored by labath on Feb 3 2017, 12:59 PM.

Details

Summary

Per discussion in D28616, having two ways two request logging (log
enable lldb XXX verbose && log enable -v lldb XXX) is confusing. This
removes the first option and standardizes all code to use the second
one.

I've added a LLDB_LOGV macro as a shorthand for if(log &&
log->GetVerbose()) and switched most of the affected log statements to
use that (I've only left a couple of cases that were doing complex
computations in an if(log) block).

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Feb 3 2017, 12:59 PM

Adding greg in case he has some thoughts on this as well.

clayborg accepted this revision.Feb 3 2017, 2:29 PM

Looks good.

This revision is now accepted and ready to land.Feb 3 2017, 2:29 PM
zturner added inline comments.Feb 3 2017, 2:32 PM
source/Target/SectionLoadList.cpp
70 ↗(On Diff #87006)

There's a subtle behavioral change here, which is that previously this line would get logged even if LIBLLDB_LOG_DYNAMIC_LOADER was not set, and now it won't. However, I've always found the GetLogIfAny / GetLogIfAll split unnecessarily confusing, so I'm not opposed to this change. Just want to point it out.

76 ↗(On Diff #87006)

No need to call AsCString().

jingham edited edge metadata.Feb 3 2017, 2:54 PM

Thanks for taking the trouble to clean that up, that's lovely!

labath added a comment.Feb 3 2017, 3:06 PM
source/Target/SectionLoadList.cpp
70 ↗(On Diff #87006)

You're right I didn't notice that. However, I suspect that this was actually not intended, so it should be fine. It'd be interesting to see how many calls to IfAny/IfAll we have with more than one category after this change.

76 ↗(On Diff #87006)

We don't have a formatter for ConstString yet. I'll interpret this as a request to add one.

clayborg added inline comments.Feb 3 2017, 3:12 PM
source/Target/SectionLoadList.cpp
70 ↗(On Diff #87006)

The previous usage was incorrect indeed. It should have been IfAll. IfAny only works when you might have something that should be logged for say "process" or for "thread". Then IfAny makes sense. Never makes sense to use IfAny with one any flag and verbose...

This is sort of a side question, but Pavel's comment brought it up. If I were new to this stuff, and wanted to know which entities had formatters and which didn't, how would I find that out easily? There are a couple of Format calls that use it, maybe they should have some doc that references where the built-in formatters are?

jingham accepted this revision.Feb 3 2017, 3:18 PM

Oh, yeah, and I should remember to check okay...

zturner edited edge metadata.Feb 3 2017, 3:24 PM

This is sort of a side question, but Pavel's comment brought it up. If I were new to this stuff, and wanted to know which entities had formatters and which didn't, how would I find that out easily? There are a couple of Format calls that use it, maybe they should have some doc that references where the built-in formatters are?

A couple ideas come to mind.

  1. Try it and see if it compiles. This won't tell you if a formatter exists and you didn't include the right header for it, though. Although if the formatter is always defined in the same header file that the object itself is defined in, this wouldn't be a problem.
  2. Grep the codebase for format_provider and FormatAdapter
labath updated this revision to Diff 87046.Feb 3 2017, 3:27 PM

Add ConstString formatter and use it.

zturner accepted this revision.Feb 3 2017, 3:31 PM

That's kind of a Meno response... How would I know to look for them if I don't know they are there. These format strings are only used in a couple of places. Those calls should tell me that I CAN use these format strings and where/how to find them.

I guess the same way you would know how to use any part of a library or API. The first time you've ever used an API, you don't know how it works, so you don't know it's capabilities. So you fiddle around, read the source code, trudge through some compiler errors, then you learned some more about how it works. Probably someone using this would try to print something, and get a compiler error that triggers a helpful static_assert which says "missing format provider for type = Foo!". That gives them the next piece of the puzzle they need to understand a little bit more. Or they look at the code for formatv to see how it works and see all the documentation.

It sounds analogous to asking "how does someone new to the codebase know that commands in LLDB are implemented by something which inherits from CommandObject?". Of course you don't if you're brand new, but once you figure it out you don't really have to spend much time thinking about it again.

FWIW, long term I would like to get rid of ALL printf style formatting. So right now, yes, they are only used in a few places. But it would be nice if it were the only type of formatting used anywhere. At which point I think most of your questions would be resolved on their own.

This revision was automatically updated to reflect the committed changes.