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
A couple of things:
- 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);
-
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…
-
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.