A couple of things:
1) This bug is copy-pasted in 3 locations. That suggests to me that this
should be raised into a function that can be fixed once. Something like:
static void printNewLineIfNecessary(Stream &S, StringRef Format);
2) I don't think the new check does what you expect either. The idea here
is to print a newline if a newline was not already part of the format
string. The patch as written will print either 2 newlines or 0 newlines.
So I think the correct fix is to change the || to an &&. However...
3) I don't think the check for `\r` is even correct at all. `\r` is not an
standard end of line character. The *two* character sequence "\r\n" is,
but that still ends with \n. I wouldn't expect that a single `\r` at the
end of a string would result in a newline. I think we should only check
for `\n`. Something like this.
static void printNewLineIfNecessary(Stream &S, StringRef Format) {
if (Format.empty() || Format.back() == '\n')
return;
S.EOL();
}
With these 3 changes combined, we can reduce those 15 lines to just a
handful, and I think it makes the code more self-documenting and easier to
understand.
On Sun, Dec 24, 2017 at 12:53 PM Michael McConville via lldb-dev <
lldb-dev@lists.llvm.org> wrote:
> In its current form, the below condition always evaluates to true. It seems
> like the author meant to use "&&" rather than "||". Someone more familiar
> with
> the code should verify this.
>
> Thanks for your time,
> Michael McConville
> University of Utah
>
> Index: Module.cpp
> ===================================================================
> --- Module.cpp (revision 321430)
> +++ Module.cpp (working copy)
> @@ -1113,7 +1113,7 @@
> const int format_len = strlen(format);
> if (format_len > 0) {
> const char last_char = format[format_len - 1];
> - if (last_char != '\n' || last_char != '\r')
> + if (last_char == '\n' || last_char == '\r')
> strm.EOL();
> }
> Host::SystemLog(Host::eSystemLogError, "%s", strm.GetData());
> @@ -1145,7 +1145,7 @@
> const int format_len = strlen(format);
> if (format_len > 0) {
> const char last_char = format[format_len - 1];
> - if (last_char != '\n' || last_char != '\r')
> + if (last_char == '\n' || last_char == '\r')
> strm.EOL();
> }
> strm.PutCString("The debug session should be aborted as the
> original "
> @@ -1171,7 +1171,7 @@
> const int format_len = strlen(format);
> if (format_len > 0) {
> const char last_char = format[format_len - 1];
> - if (last_char != '\n' || last_char != '\r')
> + if (last_char == '\n' || last_char == '\r')
> strm.EOL();
> }
> Host::SystemLog(Host::eSystemLogWarning, "%s", strm.GetData());
> _______________________________________________
> lldb-dev mailing list
> lldb-dev@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
>
_______________________________________________
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
