[Lldb-commits] [PATCH] D27632: Add Formatv() versions of all our printf style formatting functions

[Greg Clayton via Phabricator via lldb-commits]

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