labath updated this revision to Diff 84690.
labath added a comment.
Final version. No real changes, just removed usage changes in NativeProcessLinux
I will put in as sepearate patches.
https://reviews.llvm.org/D27459
Files:
include/lldb/Core/Log.h
source/Commands/CommandObjectLog.cpp
sour
labath updated this revision to Diff 84108.
labath added a comment.
- Several people expressed wishes to have this off by default, so I have done
that.
- Renamed the log option to --file-function (Note I am deliberately not
printing the line numbers -- they tend to change all the time and it's m
clayborg requested changes to this revision.
clayborg added a reviewer: clayborg.
clayborg added a comment.
This revision now requires changes to proceed.
Looks fine except I would rather not have file + line on by default.
Comment at: source/Commands/CommandObjectLog.cpp:51-52
labath updated this revision to Diff 83967.
labath added a comment.
Herald added a subscriber: mgorny.
- Replace the hacky proof-of-concept implementation with something more
production-ready
- Add a option to control adding of source information (defaulting to on)
- Add some unit tests
- Add an
labath added a comment.
I'll respond to your comments on the lldb-dev thread.
Comment at: include/lldb/Core/Log.h:218-219
+ << llvm::formatv(
\
+ "{0,-60}: ",
jasonmolenda added a comment.
A couple of thoughts / two cents.
I don't mind the current if (log) { ... } style of logging, even with the
PRIx64's and having to do filepath.GetPath().c_str() and all that. I like
being able to do extra work in a if (log) block of code -- create a
SymbolContext
Personally I'm not really a fan of source/line information at all. There
is only a very small probability that anyone using a released LLDB (e.g.
through Xcode) is going to have their log lines match up with ToT, because
we're changing files all the time. You change one thing on line 10, and
now
clayborg added a comment.
Seems weird that we are a C++ codebase and we fall back to C macros. We
currently need the macro only for file + line and to also not have to worry
about doing "if (log) log->". I am not a big fan of macros, but I am open to it
if everyone else wants it.
===
labath updated this revision to Diff 80744.
labath added a comment.
Update the example to the formatv-based API
https://reviews.llvm.org/D27459
Files:
include/lldb/Core/Error.h
include/lldb/Core/Log.h
include/lldb/Host/FileSpec.h
source/Plugins/Process/Linux/NativeProcessLinux.cpp
Inde
labath added a comment.
This discussion has diverged across lldb-dev and lldb-commits. I think we
should move it to one place. I propose lldb-dev, as that seems a better place
for high-level discussions.
https://reviews.llvm.org/D27459
___
lldb-co
jingham added a comment.
I not infrequently have some non-trivial code in the "if (log)" block that
gathers the information that I am then going to print, or loops over entities
printing them. Putting more complex code like that inside a macro looks awful
and is hard to debug. I don't think t
zturner added a comment.
In https://reviews.llvm.org/D27459#614672, @clayborg wrote:
> Does llvm::formatv allow you to specify the base of integers, or just where
> they go? Can you give it width, number base, leading characters and other
> things like a lot of what printf does?
You can speci
zturner added a comment.
In https://reviews.llvm.org/D27459#614670, @clayborg wrote:
> Part of the reason I like the current "if (log) log->Printf()" is that it
> doesn't cost you much if logging isn't enabled. So if you have a log line
> like:
>
> if (log)
> log->Printf("%s %s %s", myStr
clayborg added a comment.
I am fine with us changing logging, just want to make sure it works well.
Ideally it would include:
- almost zero cost when logging disabled. No arguments should be passed to any
functions and then determine that logging is disabled, it should check if
logging is enab
clayborg added a comment.
Does llvm::formatv allow you to specify the base of integers, or just where
they go? Can you give it width, number base, leading characters and other
things like a lot of what printf does?
https://reviews.llvm.org/D27459
clayborg added a comment.
Part of the reason I like the current "if (log) log->Printf()" is that it
doesn't cost you much if logging isn't enabled. So if you have a log line like:
if (log)
log->Printf("%s %s %s", myStringRef1.str().c_str(),
myStringRef2.str().c_str(), myStringRef3.str().c
labath added a comment.
In https://reviews.llvm.org/D27459#614549, @zturner wrote:
> A couple comments:
>
> 1. If we're going to use `formatv` (yay!) then let's standardize. No point
> mising `formatv` and streaming, especially when the former will almost always
> be less verbose.
In this ch
zturner added a comment.
A couple comments:
1. If we're going to use `formatv` (yay!) then let's standardize. No point
mising `formatv` and streaming, especially when the former will almost always
be less verbose.
2. One of the concerns that came up last time was that of evaluating the
argum
labath created this revision.
labath added a subscriber: lldb-commits.
https://reviews.llvm.org/D27459
Files:
include/lldb/Core/Log.h
source/Plugins/Process/Linux/NativeProcessLinux.cpp
Index: source/Plugins/Process/Linux/NativeProcessLinux.cpp
===
19 matches
Mail list logo
