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 'ffff' 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
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
