[Lldb-commits] [PATCH] D27632: Add Formatv() versions of all our printf style formatting functions
This revision was automatically updated to reflect the committed changes.
Closed by commit rL289922: Add methods to enable using formatv syntax in LLDB.
(authored by zturner).
Changed prior to commit:
https://reviews.llvm.org/D27632?vs=80992&id=81713#toc
Repository:
rL LLVM
https://reviews.llvm.org/D27632
Files:
lldb/trunk/include/lldb/Core/Error.h
lldb/trunk/include/lldb/Core/Log.h
lldb/trunk/include/lldb/Core/ModuleSpec.h
lldb/trunk/include/lldb/Core/Stream.h
lldb/trunk/include/lldb/Host/FileSpec.h
lldb/trunk/include/lldb/Interpreter/CommandReturnObject.h
lldb/trunk/source/Breakpoint/BreakpointOptions.cpp
lldb/trunk/source/Commands/CommandObjectApropos.cpp
lldb/trunk/source/Core/Log.cpp
lldb/trunk/source/Host/common/FileSpec.cpp
lldb/trunk/source/Symbol/ClangASTContext.cpp
lldb/trunk/source/Target/Target.cpp
lldb/trunk/unittests/Host/FileSpecTest.cpp
Index: lldb/trunk/unittests/Host/FileSpecTest.cpp
===
--- lldb/trunk/unittests/Host/FileSpecTest.cpp
+++ lldb/trunk/unittests/Host/FileSpecTest.cpp
@@ -260,3 +260,27 @@
<< "Original path: " << test.first;
}
}
+
+TEST(FileSpecTest, FormatFileSpec) {
+ auto win = FileSpec::ePathSyntaxWindows;
+
+ FileSpec F;
+ EXPECT_EQ("(empty)", llvm::formatv("{0}", F).str());
+ EXPECT_EQ("(empty)", llvm::formatv("{0:D}", F).str());
+ EXPECT_EQ("(empty)", llvm::formatv("{0:F}", F).str());
+
+ F = FileSpec("C:\\foo\\bar.txt", false, win);
+ EXPECT_EQ("C:\\foo\\bar.txt", llvm::formatv("{0}", F).str());
+ EXPECT_EQ("C:\\foo\\", llvm::formatv("{0:D}", F).str());
+ EXPECT_EQ("bar.txt", llvm::formatv("{0:F}", F).str());
+
+ F = FileSpec("foo\\bar.txt", false, win);
+ EXPECT_EQ("foo\\bar.txt", llvm::formatv("{0}", F).str());
+ EXPECT_EQ("foo\\", llvm::formatv("{0:D}", F).str());
+ EXPECT_EQ("bar.txt", llvm::formatv("{0:F}", F).str());
+
+ F = FileSpec("foo", false, win);
+ EXPECT_EQ("foo", llvm::formatv("{0}", F).str());
+ EXPECT_EQ("foo", llvm::formatv("{0:F}", F).str());
+ EXPECT_EQ("(empty)", llvm::formatv("{0:D}", F).str());
+}
\ No newline at end of file
Index: lldb/trunk/source/Target/Target.cpp
===
--- lldb/trunk/source/Target/Target.cpp
+++ lldb/trunk/source/Target/Target.cpp
@@ -1554,11 +1554,9 @@
if (load_addr == LLDB_INVALID_ADDRESS) {
ModuleSP addr_module_sp(resolved_addr.GetModule());
if (addr_module_sp && addr_module_sp->GetFileSpec())
-error.SetErrorStringWithFormat(
-"%s[0x%" PRIx64 "] can't be resolved, %s in not currently loaded",
-addr_module_sp->GetFileSpec().GetFilename().AsCString(""),
-resolved_addr.GetFileAddress(),
-addr_module_sp->GetFileSpec().GetFilename().AsCString(""));
+error.SetErrorStringWithFormatv(
+"{0:F}[{1:x+}] can't be resolved, {0:F} is not currently loaded",
+addr_module_sp->GetFileSpec(), resolved_addr.GetFileAddress());
else
error.SetErrorStringWithFormat("0x%" PRIx64 " can't be resolved",
resolved_addr.GetFileAddress());
Index: lldb/trunk/source/Symbol/ClangASTContext.cpp
===
--- lldb/trunk/source/Symbol/ClangASTContext.cpp
+++ lldb/trunk/source/Symbol/ClangASTContext.cpp
@@ -9,6 +9,9 @@
#include "lldb/Symbol/ClangASTContext.h"
+#include "llvm/Support/FormatAdapters.h"
+#include "llvm/Support/FormatVariadic.h"
+
// C Includes
// C++ Includes
#include // std::once
@@ -6739,10 +6742,7 @@
if (array) {
CompilerType element_type(getASTContext(), array->getElementType());
if (element_type.GetCompleteType()) {
- char element_name[64];
- ::snprintf(element_name, sizeof(element_name), "[%" PRIu64 "]",
- static_cast(idx));
- child_name.assign(element_name);
+ child_name = llvm::formatv("[{0}]", idx);
child_byte_size = element_type.GetByteSize(
exe_ctx ? exe_ctx->GetBestExecutionContextScope() : NULL);
child_byte_offset = (int32_t)idx * (int32_t)child_byte_size;
@@ -8883,8 +8883,8 @@
std::string base_class_type_name(base_class_qual_type.getAsString());
// Indent and print the base class type name
- s->Printf("\n%*s%s ", depth + DEPTH_INCREMENT, "",
-base_class_type_name.c_str());
+ s->Format("\n{0}{1}", llvm::fmt_repeat(" ", depth + DEPTH_INCREMENT),
+base_class_type_name);
clang::TypeInfo base_class_type_info =
getASTContext()->getTypeInfo(base_class_qual_type);
Index: lldb/trunk/source/Commands/CommandObjectApropos.cpp
===
--- lldb/trunk/source/Commands/CommandObjectApropos.cpp
+++ lldb/trunk/source/Commands/CommandObjectApropos.cpp
@@ -90
[Lldb-commits] [PATCH] D27632: Add Formatv() versions of all our printf style formatting functions
clayborg added a comment. Sounds good. Take it as my vote this is ok to start with if we get consensus https://reviews.llvm.org/D27632 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D27632: Add Formatv() versions of all our printf style formatting functions
zturner added a comment. Thanks. Not going to commit this just yet, as I know there was at least one concern over on llvm-dev about adopting this. So I'll give it another few days to see if the complaints remain. https://reviews.llvm.org/D27632 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D27632: Add Formatv() versions of all our printf style formatting functions
clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. So we need to formalize this in the formatv documentation before we begin wide adoption otherwise people can do what ever they want. It would be good to write this up so we get consistent implementations. But that is in here looks good enough to start with. https://reviews.llvm.org/D27632 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D27632: Add Formatv() versions of all our printf style formatting functions
zturner added inline comments.
Comment at: source/Host/common/FileSpec.cpp:1394
+ assert(
+ (Style.empty() || Style.equals_lower("F") || Style.equals_lower("D")) &&
+ "Invalid FileSpec style!");
clayborg wrote:
> zturner wrote:
> > clayborg wrote:
> > > If we are going to not care about case I would say we should use lower
> > > case in our initial usages of this formatter.
> > >
> > > This leads to another question: should we allow format strings to be
> > > specified with longer names and that all formatv strings can be minimally
> > > specified like the LLDB command names? Then we can choose more
> > > descriptive names like "filename" and "directory" which can be shortened
> > > to "f" and "d" since they minimally match. The main issue with this is if
> > > you wanted to add another format flavor that starts with any letter that
> > > already matches, like say we wanted to later add "filename-no-extension".
> > > We could say that the first in the list that partially matches would
> > > always win. This would mean that when adding support for a new flavor you
> > > would always need to add it to the end of the list. This can help keeps
> > > things more readable in the ::format functions while allowing users to
> > > also be explicit if they need to.
> > >
> > > It would be nice to avoid asserts if possible in the logging ::format
> > > code as if we ever open up the logging to users somehow (uncontrolled
> > > input), we don't want to crash. I would rather see something in the
> > > output like "invalid format style '' for lldb_private::FileSpec" just
> > > to be safe. The user will see this output and it would be nice not to
> > > crash.
> > I'm sort of ambivalent about long option names in the way you suggest. It
> > definitely allows for less cryptic format strings, but on the other hand
> > with our 80-column limit, with run the risk of leading to additional levels
> > of wrapping which could reduce readability. That said, it's easy to modify
> > these formatters, so maybe we could start with short only, and then
> > re-evaluate after it starts getting some usage? Would give people a chance
> > to build up a stronger opinion one way or the other.
> >
> > As for asserts, I don't think it should be an issue here. The reason I say
> > this is because the assert is compiled out in release mode, so the assert
> > itself will never be the cause of a crash for the user. The only way it
> > could crash is if the code that followed did not perform gracefully under
> > the condition that triggered the assert to fire in the first place.
> >
> > The assert here is used only to notify the programmer that they've made a
> > mistake in their debug build, but in a normal build it will fall back to
> > using the default Dir + Separator + Filename format if anything in the
> > format string is unrecognized.
> >
> > I prefer this to adding additional conditional branches, because printing
> > something sane is better than printing "Unknown Format String" in a log
> > file.
> >
> >
> I am saying that we add long format name support in now only in the
> ::format() functions but use them with the minimal characters on our code
> when logging. This would avoid any 80 char limitations when using them, but
> keep the ::format() functions more readable.
>
> I am fine with the assert being there if the code still is designed to
> function when the assert isn't there. LLVM tends to say asserts are
> invariants and the code isn't expect to function when the asserts are off, so
> as long as we avoid avoid this in LLDB I am fine (use asserts for alerting,
> but code around it).
Ahh I see what you mean. I wouldn't want us to do anything like that across
the board, because we can't foresee all possible ways we might want to use it,
and I don't think we'd be able to apply this rule consistently across all types
and all formatters we'd ever make in LLDB. For example, I can imagine a
hypothetical situation where you might do `llvm::formatv("{0:pxest}") where p,
x, e, s, and t are "codes" for some specific field, and it just strings all the
results together. Then there are some situations where case matters, like when
printing integers `X` means hex with uppercase digits and `x` means hex with
lowercase digits.
https://reviews.llvm.org/D27632
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D27632: Add Formatv() versions of all our printf style formatting functions
clayborg added inline comments.
Comment at: source/Host/common/FileSpec.cpp:1394
+ assert(
+ (Style.empty() || Style.equals_lower("F") || Style.equals_lower("D")) &&
+ "Invalid FileSpec style!");
zturner wrote:
> clayborg wrote:
> > If we are going to not care about case I would say we should use lower case
> > in our initial usages of this formatter.
> >
> > This leads to another question: should we allow format strings to be
> > specified with longer names and that all formatv strings can be minimally
> > specified like the LLDB command names? Then we can choose more descriptive
> > names like "filename" and "directory" which can be shortened to "f" and
> > "d" since they minimally match. The main issue with this is if you wanted
> > to add another format flavor that starts with any letter that already
> > matches, like say we wanted to later add "filename-no-extension". We could
> > say that the first in the list that partially matches would always win.
> > This would mean that when adding support for a new flavor you would always
> > need to add it to the end of the list. This can help keeps things more
> > readable in the ::format functions while allowing users to also be explicit
> > if they need to.
> >
> > It would be nice to avoid asserts if possible in the logging ::format code
> > as if we ever open up the logging to users somehow (uncontrolled input), we
> > don't want to crash. I would rather see something in the output like
> > "invalid format style '' for lldb_private::FileSpec" just to be safe.
> > The user will see this output and it would be nice not to crash.
> I'm sort of ambivalent about long option names in the way you suggest. It
> definitely allows for less cryptic format strings, but on the other hand with
> our 80-column limit, with run the risk of leading to additional levels of
> wrapping which could reduce readability. That said, it's easy to modify
> these formatters, so maybe we could start with short only, and then
> re-evaluate after it starts getting some usage? Would give people a chance
> to build up a stronger opinion one way or the other.
>
> As for asserts, I don't think it should be an issue here. The reason I say
> this is because the assert is compiled out in release mode, so the assert
> itself will never be the cause of a crash for the user. The only way it
> could crash is if the code that followed did not perform gracefully under the
> condition that triggered the assert to fire in the first place.
>
> The assert here is used only to notify the programmer that they've made a
> mistake in their debug build, but in a normal build it will fall back to
> using the default Dir + Separator + Filename format if anything in the format
> string is unrecognized.
>
> I prefer this to adding additional conditional branches, because printing
> something sane is better than printing "Unknown Format String" in a log file.
>
>
I am saying that we add long format name support in now only in the ::format()
functions but use them with the minimal characters on our code when logging.
This would avoid any 80 char limitations when using them, but keep the
::format() functions more readable.
I am fine with the assert being there if the code still is designed to function
when the assert isn't there. LLVM tends to say asserts are invariants and the
code isn't expect to function when the asserts are off, so as long as we avoid
avoid this in LLDB I am fine (use asserts for alerting, but code around it).
Comment at: source/Host/common/FileSpec.cpp:1401
+ if (dir.empty() && file.empty()) {
+Stream << "(empty)";
+return;
Do we want all strings and classes that don't have valid state to print
"(empty)"? If so maybe we build in something into formatv like:
```
Stream << formatv::empty();
```
This would allow us to get a consistent output from everyone when their objects
are invalid.
https://reviews.llvm.org/D27632
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D27632: Add Formatv() versions of all our printf style formatting functions
zturner added inline comments.
Comment at: source/Host/common/FileSpec.cpp:1394
+ assert(
+ (Style.empty() || Style.equals_lower("F") || Style.equals_lower("D")) &&
+ "Invalid FileSpec style!");
clayborg wrote:
> If we are going to not care about case I would say we should use lower case
> in our initial usages of this formatter.
>
> This leads to another question: should we allow format strings to be
> specified with longer names and that all formatv strings can be minimally
> specified like the LLDB command names? Then we can choose more descriptive
> names like "filename" and "directory" which can be shortened to "f" and "d"
> since they minimally match. The main issue with this is if you wanted to add
> another format flavor that starts with any letter that already matches, like
> say we wanted to later add "filename-no-extension". We could say that the
> first in the list that partially matches would always win. This would mean
> that when adding support for a new flavor you would always need to add it to
> the end of the list. This can help keeps things more readable in the ::format
> functions while allowing users to also be explicit if they need to.
>
> It would be nice to avoid asserts if possible in the logging ::format code as
> if we ever open up the logging to users somehow (uncontrolled input), we
> don't want to crash. I would rather see something in the output like "invalid
> format style '' for lldb_private::FileSpec" just to be safe. The user
> will see this output and it would be nice not to crash.
I'm sort of ambivalent about long option names in the way you suggest. It
definitely allows for less cryptic format strings, but on the other hand with
our 80-column limit, with run the risk of leading to additional levels of
wrapping which could reduce readability. That said, it's easy to modify these
formatters, so maybe we could start with short only, and then re-evaluate after
it starts getting some usage? Would give people a chance to build up a
stronger opinion one way or the other.
As for asserts, I don't think it should be an issue here. The reason I say
this is because the assert is compiled out in release mode, so the assert
itself will never be the cause of a crash for the user. The only way it could
crash is if the code that followed did not perform gracefully under the
condition that triggered the assert to fire in the first place.
The assert here is used only to notify the programmer that they've made a
mistake in their debug build, but in a normal build it will fall back to using
the default Dir + Separator + Filename format if anything in the format string
is unrecognized.
I prefer this to adding additional conditional branches, because printing
something sane is better than printing "Unknown Format String" in a log file.
https://reviews.llvm.org/D27632
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D27632: Add Formatv() versions of all our printf style formatting functions
clayborg added a comment.
Looking good. A few little changes and possibly supporting long option names
that can be minimally specified. See inlined comments.
Comment at: source/Host/common/FileSpec.cpp:1394
+ assert(
+ (Style.empty() || Style.equals_lower("F") || Style.equals_lower("D")) &&
+ "Invalid FileSpec style!");
If we are going to not care about case I would say we should use lower case in
our initial usages of this formatter.
This leads to another question: should we allow format strings to be specified
with longer names and that all formatv strings can be minimally specified like
the LLDB command names? Then we can choose more descriptive names like
"filename" and "directory" which can be shortened to "f" and "d" since they
minimally match. The main issue with this is if you wanted to add another
format flavor that starts with any letter that already matches, like say we
wanted to later add "filename-no-extension". We could say that the first in the
list that partially matches would always win. This would mean that when adding
support for a new flavor you would always need to add it to the end of the
list. This can help keeps things more readable in the ::format functions while
allowing users to also be explicit if they need to.
It would be nice to avoid asserts if possible in the logging ::format code as
if we ever open up the logging to users somehow (uncontrolled input), we don't
want to crash. I would rather see something in the output like "invalid format
style '' for lldb_private::FileSpec" just to be safe. The user will see
this output and it would be nice not to crash.
Comment at: source/Target/Target.cpp:1558
+error.SetErrorStringWithFormatv(
+"{0:F}[{1:x+}] can't be resolved, {0:F} is not currently loaded",
+addr_module_sp->GetFileSpec(), resolved_addr.GetFileAddress());
I would prefer us to use lower case format characters in our example code if
possible, so 'f' instead of 'F'.
https://reviews.llvm.org/D27632
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D27632: Add Formatv() versions of all our printf style formatting functions
On 10 December 2016 at 18:09, Zachary Turner wrote:
> To elaborate about member function vs format provider, decoupling types from
> the way they're formatted seems like a good design practice in general. For
> example, you might want to add support for formatting a class which you
> don't have the code to. Sure, you can do it one way whenever possible and
> another way whenever it's not possible, but since tight coupling leads to
> more fragile code as a general rule, it seems better to prefer the loose
> coupling whenever possible.
Thanks for explaining this. Loose coupling sounds very reasonable.
Reading the docs again, they do seem to imply that this is the
preferred method. I think what confused me is that I started off with
the source code, and that one searches for the member function first.
:)
>
> On the other hand, there are certain times when you *must* use a
> member-based formatter. For example, when something already has a formatter
> defined for it (either via member syntax or format_provider specialization)
> and you want to change the behavior. A good example of this is the
> `fmt_repeat` adaptor I use in this patch. int already has a formatter
> defined for it, so if we want to repeat an int 10 times, we have no way to
> do that. So we define a class `fmt_repeat` and give that a format member
> function, then instead of saying formatv("{0}", 7) we say formatv("{0}",
> fmt_repeat(7, 10));
In this case, I have a feeling that the search for a member function
was implemented in a more complicated way than necessary (and I have
made it more complicated without understanding the reasoning behind
it). But that's a different discussion. I'll try to send you a patch
simplifying it.
> Another point regarding F and D styles vs F.GetFilename() and
> F.GetDirectory(). One of the original design goals was brevity, because
> usually brevity helps readability. To that end, I find
>
> formatv("Dir: {0:D}, File: {0:F}", F);
>
> more readable than
>
> formatv("Dir: {0}, File: {1}, F.GetDirectory(), F.GetFilename());
>
> There's another, more subtle, advantage though. If you just use
> GetFilename() and GetDirectory(), then what do you print if the string is
> empty? Here we've got a simple string, no different from any other string,
> and an empty string is perfectly normal. With specific types such as
> FileSpec, you know more about where that string came from, so you can make
> better decisions about how you want to handle the empty case (this argument
> applies to anything, not just strings, btw). So now you either have to
> modify the core string formatter so that you can do something like this to
> specify an "empty" value:
>
> formatv("Dir: {0:(empty)}, File: {1:(empty)}", File.GetDirectory(),
> File.GetFilename());
>
> or, even worse, you have to do this:
>
> formatv("Dir: {0}, File: {1}, File.GetFilename().IsEmpty() ? "(empty)" :
> File.GetFilename(), F.GetDirectory().IsEmpty() ? "(empty)" :
> F.GetDirectory());
>
> both are getting further and further away from the very simple and easy to
> read
>
> formatv("Dir: {0:D}, File: {0:F}", F);
>
> and both increase the possibility for errors and inconsistencies throughout
> the code whereas with the F and D styles, you guarantee that every time you
> ever print a FileSpec, it's always going to be consistent (of course, we
> could add an override mechanism to the FileSpec formatter as well if it were
> *really* necessary, but it seems best to try to avoid it as much as
> possible)
>
Seems reasonable. I withdraw my objection. LGTM.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D27632: Add Formatv() versions of all our printf style formatting functions
To elaborate about member function vs format provider, decoupling types
from the way they're formatted seems like a good design practice in
general. For example, you might want to add support for formatting a class
which you don't have the code to. Sure, you can do it one way whenever
possible and another way whenever it's not possible, but since tight
coupling leads to more fragile code as a general rule, it seems better to
prefer the loose coupling whenever possible.
On the other hand, there are certain times when you *must* use a
member-based formatter. For example, when something already has a
formatter defined for it (either via member syntax or format_provider
specialization) and you want to change the behavior. A good example of
this is the `fmt_repeat` adaptor I use in this patch. int already has a
formatter defined for it, so if we want to repeat an int 10 times, we have
no way to do that. So we define a class `fmt_repeat` and give that a
format member function, then instead of saying formatv("{0}", 7) we say
formatv("{0}", fmt_repeat(7, 10));
Another point regarding F and D styles vs F.GetFilename() and
F.GetDirectory(). One of the original design goals was brevity, because
usually brevity helps readability. To that end, I find
formatv("Dir: {0:D}, File: {0:F}", F);
more readable than
formatv("Dir: {0}, File: {1}, F.GetDirectory(), F.GetFilename());
There's another, more subtle, advantage though. If you just use
GetFilename() and GetDirectory(), then what do you print if the string is
empty? Here we've got a simple string, no different from any other string,
and an empty string is perfectly normal. With specific types such as
FileSpec, you know more about where that string came from, so you can make
better decisions about how you want to handle the empty case (this argument
applies to anything, not just strings, btw). So now you either have to
modify the core string formatter so that you can do something like this to
specify an "empty" value:
formatv("Dir: {0:(empty)}, File: {1:(empty)}", File.GetDirectory(), File.
GetFilename());
or, even worse, you have to do this:
formatv("Dir: {0}, File: {1}, File.GetFilename().IsEmpty() ? "(empty)" :
File.GetFilename(), F.GetDirectory().IsEmpty() ? "(empty)" :
F.GetDirectory());
both are getting further and further away from the very simple and easy to
read
formatv("Dir: {0:D}, File: {0:F}", F);
and both increase the possibility for errors and inconsistencies throughout
the code whereas with the F and D styles, you guarantee that every time you
ever print a FileSpec, it's always going to be consistent (of course, we
could add an override mechanism to the FileSpec formatter as well if it
were *really* necessary, but it seems best to try to avoid it as much as
possible)
On Sat, Dec 10, 2016 at 4:51 AM Zachary Turner wrote:
> I would say format provider should be the default way to add formatting
> for a commonly formatted type. Member function syntax is used to implement
> adaptors that allow you to override formatting behavior of a type that
> already has a provider.
>
> One nice thing about F and D would be this:
>
> formatv("Dir: {0:D}, File: {0:F}", F):
>
> Here you only pass the argument once, but format it differently each time.
> You do have a point about not getting a compiler error. In the future i had
> planned to add compile time format string checking but it's a ways out. For
> now we assert and then fallback to the default case of an empty style.
>
> Also, i still haven't finished the Args stuff, so no risk of me going on a
> reformatting spree :)
> On Sat, Dec 10, 2016 at 1:03 AM Pavel Labath via Phabricator <
> revi...@reviews.llvm.org> wrote:
>
> labath added a comment.
>
> This looks like a step in the right direction. I trust you won't go on a
> reformatting spree of the log messages until we settle on the syntax there.
> ;)
>
>
>
>
> Comment at: include/lldb/Core/ModuleSpec.h:244
> + strm.Format("object_mod_time = {0:x+}",
>uint64_t(llvm::sys::toTimeT(m_object_mod_time)));
> }
>
> you can delete the `uint64_t` cast now. It was only there to make this
> portably printable.
>
>
>
> Comment at: include/lldb/Host/FileSpec.h:784
> +///
> +template <> struct format_provider {
> + static void format(const lldb_private::FileSpec &F, llvm::raw_ostream
> &Stream,
>
> I am wondering.. what is the preferred style for adding formatting
> capability. I sort of assumed that the `format_provider` thingy was
> reserved for cases where you had no control over the object being formatted
> (e.g. because it comes from a third party library), and that for cases like
> this we would use the member format function (or, to put it differently: if
> we're going to be using the `format_provider` everywhere, what's the point
> of having the member functions in the first place?).
>
> Also, my preference would be to not have the F and D styles for
Re: [Lldb-commits] [PATCH] D27632: Add Formatv() versions of all our printf style formatting functions
I would say format provider should be the default way to add formatting for
a commonly formatted type. Member function syntax is used to implement
adaptors that allow you to override formatting behavior of a type that
already has a provider.
One nice thing about F and D would be this:
formatv("Dir: {0:D}, File: {0:F}", F):
Here you only pass the argument once, but format it differently each time.
You do have a point about not getting a compiler error. In the future i had
planned to add compile time format string checking but it's a ways out. For
now we assert and then fallback to the default case of an empty style.
Also, i still haven't finished the Args stuff, so no risk of me going on a
reformatting spree :)
On Sat, Dec 10, 2016 at 1:03 AM Pavel Labath via Phabricator <
revi...@reviews.llvm.org> wrote:
> labath added a comment.
>
> This looks like a step in the right direction. I trust you won't go on a
> reformatting spree of the log messages until we settle on the syntax there.
> ;)
>
>
>
>
> Comment at: include/lldb/Core/ModuleSpec.h:244
> + strm.Format("object_mod_time = {0:x+}",
>uint64_t(llvm::sys::toTimeT(m_object_mod_time)));
> }
>
> you can delete the `uint64_t` cast now. It was only there to make this
> portably printable.
>
>
>
> Comment at: include/lldb/Host/FileSpec.h:784
> +///
> +template <> struct format_provider {
> + static void format(const lldb_private::FileSpec &F, llvm::raw_ostream
> &Stream,
>
> I am wondering.. what is the preferred style for adding formatting
> capability. I sort of assumed that the `format_provider` thingy was
> reserved for cases where you had no control over the object being formatted
> (e.g. because it comes from a third party library), and that for cases like
> this we would use the member format function (or, to put it differently: if
> we're going to be using the `format_provider` everywhere, what's the point
> of having the member functions in the first place?).
>
> Also, my preference would be to not have the F and D styles for FileSpec.
> I think they are redundant, as we have FileSpec::GetFilename and
> FileSpec::GetDirectory, and also they are not compiler-enforced --
> `Formatv('{0}', fs.GetFilename())` is slightly longer than
> `Formatv('{0:F}', fs)`, but if I make a typo in the former one, it will be
> a compiler error, while the other one won't. I suppose it could be useful,
> if one wants to format say an array of FileSpecs using the array format
> syntax, but I'm not sure how often will someone want to do that *and*
> display only the filenames.
>
> I don't care strongly about any of these things, but I want to know what
> to expect.
>
>
> https://reviews.llvm.org/D27632
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D27632: Add Formatv() versions of all our printf style formatting functions
labath added a comment.
This looks like a step in the right direction. I trust you won't go on a
reformatting spree of the log messages until we settle on the syntax there. ;)
Comment at: include/lldb/Core/ModuleSpec.h:244
+ strm.Format("object_mod_time = {0:x+}",
uint64_t(llvm::sys::toTimeT(m_object_mod_time)));
}
you can delete the `uint64_t` cast now. It was only there to make this portably
printable.
Comment at: include/lldb/Host/FileSpec.h:784
+///
+template <> struct format_provider {
+ static void format(const lldb_private::FileSpec &F, llvm::raw_ostream
&Stream,
I am wondering.. what is the preferred style for adding formatting capability.
I sort of assumed that the `format_provider` thingy was reserved for cases
where you had no control over the object being formatted (e.g. because it comes
from a third party library), and that for cases like this we would use the
member format function (or, to put it differently: if we're going to be using
the `format_provider` everywhere, what's the point of having the member
functions in the first place?).
Also, my preference would be to not have the F and D styles for FileSpec. I
think they are redundant, as we have FileSpec::GetFilename and
FileSpec::GetDirectory, and also they are not compiler-enforced --
`Formatv('{0}', fs.GetFilename())` is slightly longer than `Formatv('{0:F}',
fs)`, but if I make a typo in the former one, it will be a compiler error,
while the other one won't. I suppose it could be useful, if one wants to format
say an array of FileSpecs using the array format syntax, but I'm not sure how
often will someone want to do that *and* display only the filenames.
I don't care strongly about any of these things, but I want to know what to
expect.
https://reviews.llvm.org/D27632
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D27632: Add Formatv() versions of all our printf style formatting functions
zturner updated this revision to Diff 80992.
zturner added a comment.
Added a format provider for `FileSpec`. Style syntax is documented in this
patch. Added some unit tests so you can see the output.
To answer Greg's earlier question about printing c-strings, `formatv("'{0}'
'{1}' '{2}'", (const char *)nullptr, "", "test");` would print "'' '' 'test'";.
So yes that means that nullptr doesn't print "(null)". It could be made to do
so if desired, but if it's any consolation the goal is really to move away from
c-strings, and StringRef doesn't have any concept of null. There's just empty
and not empty.
One nice thing about is that you can easily change the behavior of existing
formatters using a mechanism which I've called "adapters". You can see an
example of a builtin adapter in this patch, where I use `fmt_repeat` to repeat
a character 7 times. To write an adapter for const char * that prints (null),
you could do this:
struct fmt_or_null {
explicit fmt_or_null(const char *s) : s(s) {}
void format(llvm::raw_ostream &Stream, StringRef Style) const {
if (!s)
Stream << "(null)"; // Override the default behavior for nullptr;
else
llvm::format_provider::format(s, Stream, Style); //
Otherwise just use the default;
}
const char *s;
};
void foo() {
const char *s = nullptr;
std::string result = llvm::formatv("{0}", fmt_or_null(s));
}
https://reviews.llvm.org/D27632
Files:
include/lldb/Core/Error.h
include/lldb/Core/Log.h
include/lldb/Core/ModuleSpec.h
include/lldb/Core/Stream.h
include/lldb/Host/FileSpec.h
include/lldb/Interpreter/CommandReturnObject.h
source/Breakpoint/BreakpointOptions.cpp
source/Commands/CommandObjectApropos.cpp
source/Core/Log.cpp
source/Host/common/FileSpec.cpp
source/Symbol/ClangASTContext.cpp
source/Target/Target.cpp
unittests/Host/FileSpecTest.cpp
Index: unittests/Host/FileSpecTest.cpp
===
--- unittests/Host/FileSpecTest.cpp
+++ unittests/Host/FileSpecTest.cpp
@@ -260,3 +260,27 @@
<< "Original path: " << test.first;
}
}
+
+TEST(FileSpecTest, FormatFileSpec) {
+ auto win = FileSpec::ePathSyntaxWindows;
+
+ FileSpec F;
+ EXPECT_EQ("(empty)", llvm::formatv("{0}", F).str());
+ EXPECT_EQ("(empty)", llvm::formatv("{0:D}", F).str());
+ EXPECT_EQ("(empty)", llvm::formatv("{0:F}", F).str());
+
+ F = FileSpec("C:\\foo\\bar.txt", false, win);
+ EXPECT_EQ("C:\\foo\\bar.txt", llvm::formatv("{0}", F).str());
+ EXPECT_EQ("C:\\foo\\", llvm::formatv("{0:D}", F).str());
+ EXPECT_EQ("bar.txt", llvm::formatv("{0:F}", F).str());
+
+ F = FileSpec("foo\\bar.txt", false, win);
+ EXPECT_EQ("foo\\bar.txt", llvm::formatv("{0}", F).str());
+ EXPECT_EQ("foo\\", llvm::formatv("{0:D}", F).str());
+ EXPECT_EQ("bar.txt", llvm::formatv("{0:F}", F).str());
+
+ F = FileSpec("foo", false, win);
+ EXPECT_EQ("foo", llvm::formatv("{0}", F).str());
+ EXPECT_EQ("foo", llvm::formatv("{0:F}", F).str());
+ EXPECT_EQ("(empty)", llvm::formatv("{0:D}", F).str());
+}
\ No newline at end of file
Index: source/Target/Target.cpp
===
--- source/Target/Target.cpp
+++ source/Target/Target.cpp
@@ -1554,11 +1554,9 @@
if (load_addr == LLDB_INVALID_ADDRESS) {
ModuleSP addr_module_sp(resolved_addr.GetModule());
if (addr_module_sp && addr_module_sp->GetFileSpec())
-error.SetErrorStringWithFormat(
-"%s[0x%" PRIx64 "] can't be resolved, %s in not currently loaded",
-addr_module_sp->GetFileSpec().GetFilename().AsCString(""),
-resolved_addr.GetFileAddress(),
-addr_module_sp->GetFileSpec().GetFilename().AsCString(""));
+error.SetErrorStringWithFormatv(
+"{0:F}[{1:x+}] can't be resolved, {0:F} is not currently loaded",
+addr_module_sp->GetFileSpec(), resolved_addr.GetFileAddress());
else
error.SetErrorStringWithFormat("0x%" PRIx64 " can't be resolved",
resolved_addr.GetFileAddress());
Index: source/Symbol/ClangASTContext.cpp
===
--- source/Symbol/ClangASTContext.cpp
+++ source/Symbol/ClangASTContext.cpp
@@ -9,6 +9,9 @@
#include "lldb/Symbol/ClangASTContext.h"
+#include "llvm/Support/FormatAdapters.h"
+#include "llvm/Support/FormatVariadic.h"
+
// C Includes
// C++ Includes
#include // std::once
@@ -6739,10 +6742,7 @@
if (array) {
CompilerType element_type(getASTContext(), array->getElementType());
if (element_type.GetCompleteType()) {
- char element_name[64];
- ::snprintf(element_name, sizeof(element_name), "[%" PRIu64 "]",
- static_cast(idx));
- child_name.assign(element_name);
+ child_name = llvm::formatv("[{0}]", idx);
child_byte_size = element_type.GetByteSize(
Re: [Lldb-commits] [PATCH] D27632: Add Formatv() versions of all our printf style formatting functions
Seems reasonable, I'll make a FileSpec formatter and update the patch On Fri, Dec 9, 2016 at 4:03 PM Greg Clayton via Phabricator < revi...@reviews.llvm.org> wrote: > clayborg requested changes to this revision. > clayborg added a comment. > This revision now requires changes to proceed. > > I would like to see the FileSpec having its formatv stuff done for this > since it will start a whole slew of people wanting to use it and we should > have an example of a class that show how to add your own formatting options. > > > https://reviews.llvm.org/D27632 > > > > ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D27632: Add Formatv() versions of all our printf style formatting functions
clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. I would like to see the FileSpec having its formatv stuff done for this since it will start a whole slew of people wanting to use it and we should have an example of a class that show how to add your own formatting options. https://reviews.llvm.org/D27632 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D27632: Add Formatv() versions of all our printf style formatting functions
clayborg added a comment.
It is nice to be able to use StringRef and std::string as is. I was wondering
if you have:
StringRef null;
StringRef empty("");
StringRef hello("hello")
What does this show:
formatv("{0} {1} {2}", null, empty, hello);
I would hope for:
(null) "" "hello"
Where "(null)" is used for the empty StringRef since it doesn't have a value.
This is what printf does if you pass nullptr in for a %s. The other string has
a pointer, but no size, so it should show the empty string. Also for
std::string and for StringRef if would be great if we can show binary values
since a StringRef and std::string can contain NULL characters and also non
printable characters. One option that would be really cool for both StringRef
and std::string would be the memory view stuff we did a month back! So we could
do:
StringRef bytes("\x01\x02\x03");
formatv("{0:memory}", bytes);
And get out a memory dump.
Comment at: source/Target/Target.cpp:1558-1559
+error.SetErrorStringWithFormatv(
+"{0}[{1:x+}] can't be resolved, {0} is not currently loaded",
addr_module_sp->GetFileSpec().GetFilename().AsCString(""),
+resolved_addr.GetFileAddress());
Can we add lldb_private::FileSpec support with this patch as well? Then the
code can be:
```
error.SetErrorStringWithFormatv(
"{0}[{1:x+}] can't be resolved, {0;filename} is not currently
loaded",
addr_module_sp->GetFileSpec(), resolved_addr.GetFileAddress());
```
https://reviews.llvm.org/D27632
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D27632: Add Formatv() versions of all our printf style formatting functions
zturner created this revision.
zturner added reviewers: labath, clayborg, jingham.
zturner added a subscriber: lldb-commits.
We have various functions like `Stream::Printf()`, and
`Error::SetErrorStringWithFormat()`, `Log::Printf()`, and various others. I
added functions that delegate to `formatv` in case people want to see some
actual usage in practice.
I also converted a few various Printfs, etc to this syntax just to demonstrate
what it looks like. I tried to find callsites that would illustrate how it
makes things simpler, such as :
- not needing to call `.c_str()` and being able to pass `std::string` and/or
`StringRef` directly as the argument.
- places where the same argument is printed multiple times, only having to pass
it once (incidentally this fixed a spelling mistake since when passing the
argument the 2nd time, there had been a typo. So this also serves as an
example of how this is safer than Printf, since less code typed = fewer chances
to mess up).
- places where complex format strings are constructed, such as using the printf
width specifier `*` or `PRIx64` are used and now we can simply pass whatever
type we have directly, even if it's architecture dependent like `size_t`.
Various other uses.
Obviously a change like this is too big to do all in one pass, since it would
have a roughly 100% chance of introducing new bugs and be impossible to bisect.
This is basically just to get the hookups in place and have a single-digit
number of illustrative use cases.
https://reviews.llvm.org/D27632
Files:
include/lldb/Core/Error.h
include/lldb/Core/Log.h
include/lldb/Core/ModuleSpec.h
include/lldb/Core/Stream.h
include/lldb/Interpreter/CommandReturnObject.h
source/Breakpoint/BreakpointOptions.cpp
source/Commands/CommandObjectApropos.cpp
source/Core/Log.cpp
source/Symbol/ClangASTContext.cpp
source/Target/Target.cpp
Index: source/Target/Target.cpp
===
--- source/Target/Target.cpp
+++ source/Target/Target.cpp
@@ -1554,11 +1554,10 @@
if (load_addr == LLDB_INVALID_ADDRESS) {
ModuleSP addr_module_sp(resolved_addr.GetModule());
if (addr_module_sp && addr_module_sp->GetFileSpec())
-error.SetErrorStringWithFormat(
-"%s[0x%" PRIx64 "] can't be resolved, %s in not currently loaded",
+error.SetErrorStringWithFormatv(
+"{0}[{1:x+}] can't be resolved, {0} is not currently loaded",
addr_module_sp->GetFileSpec().GetFilename().AsCString(""),
-resolved_addr.GetFileAddress(),
-addr_module_sp->GetFileSpec().GetFilename().AsCString(""));
+resolved_addr.GetFileAddress());
else
error.SetErrorStringWithFormat("0x%" PRIx64 " can't be resolved",
resolved_addr.GetFileAddress());
Index: source/Symbol/ClangASTContext.cpp
===
--- source/Symbol/ClangASTContext.cpp
+++ source/Symbol/ClangASTContext.cpp
@@ -9,6 +9,9 @@
#include "lldb/Symbol/ClangASTContext.h"
+#include "llvm/Support/FormatAdapters.h"
+#include "llvm/Support/FormatVariadic.h"
+
// C Includes
// C++ Includes
#include // std::once
@@ -6739,10 +6742,7 @@
if (array) {
CompilerType element_type(getASTContext(), array->getElementType());
if (element_type.GetCompleteType()) {
- char element_name[64];
- ::snprintf(element_name, sizeof(element_name), "[%" PRIu64 "]",
- static_cast(idx));
- child_name.assign(element_name);
+ child_name = llvm::formatv("[{0}]", idx);
child_byte_size = element_type.GetByteSize(
exe_ctx ? exe_ctx->GetBestExecutionContextScope() : NULL);
child_byte_offset = (int32_t)idx * (int32_t)child_byte_size;
@@ -8883,8 +8883,8 @@
std::string base_class_type_name(base_class_qual_type.getAsString());
// Indent and print the base class type name
- s->Printf("\n%*s%s ", depth + DEPTH_INCREMENT, "",
-base_class_type_name.c_str());
+ s->Format("\n{0}{1}", llvm::fmt_repeat(" ", depth + DEPTH_INCREMENT),
+base_class_type_name);
clang::TypeInfo base_class_type_info =
getASTContext()->getTypeInfo(base_class_qual_type);
Index: source/Core/Log.cpp
===
--- source/Core/Log.cpp
+++ source/Core/Log.cpp
@@ -138,20 +138,6 @@
}
//--
-// Print debug strings if and only if the global debug option is set to
-// a non-zero value.
-//--
-void Log::DebugVerbose(const char *format, ...) {
- if (!GetOptions().AllSet(LLDB_LOG_OPTION_DEBUG | LLDB_LOG_OPTION_VERBOSE))
-return;
-
- va_list args;
- va_start(args, for
