<div dir="ltr"><div>A couple of things:</div><div><br></div><div>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:</div><div><br></div><div>static void printNewLineIfNecessary(Stream &S, StringRef Format);</div><div><br></div><div>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...</div><div><br></div><div>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.</div><div><br></div><div>static void printNewLineIfNecessary(Stream &S, StringRef Format) {</div><div>  if (Format.empty() || Format.back() == '\n')</div><div>    return;</div><div>  S.EOL();<br></div><div>}</div><div><br></div><div>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.</div><div><br></div><div class="gmail_quote"><div dir="ltr">On Sun, Dec 24, 2017 at 12:53 PM Michael McConville via lldb-dev <<a href="mailto:lldb-dev@lists.llvm.org" target="_blank">lldb-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">In its current form, the below condition always evaluates to true. It seems<br>
like the author meant to use "&&" rather than "||". Someone more familiar with<br>
the code should verify this.<br>
<br>
Thanks for your time,<br>
Michael McConville<br>
University of Utah<br>
<br>
Index: Module.cpp<br>
===================================================================<br>
--- Module.cpp  (revision 321430)<br>
+++ Module.cpp  (working copy)<br>
@@ -1113,7 +1113,7 @@<br>
     const int format_len = strlen(format);<br>
     if (format_len > 0) {<br>
       const char last_char = format[format_len - 1];<br>
-      if (last_char != '\n' || last_char != '\r')<br>
+      if (last_char == '\n' || last_char == '\r')<br>
         strm.EOL();<br>
     }<br>
     Host::SystemLog(Host::eSystemLogError, "%s", strm.GetData());<br>
@@ -1145,7 +1145,7 @@<br>
         const int format_len = strlen(format);<br>
         if (format_len > 0) {<br>
           const char last_char = format[format_len - 1];<br>
-          if (last_char != '\n' || last_char != '\r')<br>
+          if (last_char == '\n' || last_char == '\r')<br>
             strm.EOL();<br>
         }<br>
         strm.PutCString("The debug session should be aborted as the original "<br>
@@ -1171,7 +1171,7 @@<br>
     const int format_len = strlen(format);<br>
     if (format_len > 0) {<br>
       const char last_char = format[format_len - 1];<br>
-      if (last_char != '\n' || last_char != '\r')<br>
+      if (last_char == '\n' || last_char == '\r')<br>
         strm.EOL();<br>
     }<br>
     Host::SystemLog(Host::eSystemLogWarning, "%s", strm.GetData());<br>
_______________________________________________<br>
lldb-dev mailing list<br>
<a href="mailto:lldb-dev@lists.llvm.org" target="_blank">lldb-dev@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev</a><br>
</blockquote></div></div>