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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
