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

[Zachary Turner via Phabricator via lldb-commits]

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 'ffff' 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