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