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

[Greg Clayton via Phabricator via lldb-commits]

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