[lldb-dev] [PATCH] Fix boolean typo

[Zachary Turner via lldb-dev]

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 at 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 at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-dev/attachments/20171224/ed12a29f/attachment.html>