-
Notifications
You must be signed in to change notification settings - Fork 91
fix #337 - line splicing in comment not handled properly #431
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Here are the corresponding links about this issue:
|
The PR title is misleading. The fix is to handle line splicing differently. simplecpp doesn't contain any uninitvar checking and there is no uninitialized variable usage I hope. #337 is much more proper. |
@@ -788,6 +788,35 @@ void simplecpp::TokenList::readfile(Stream &stream, const std::string &filename, | |||
while (stream.good() && ch != '\r' && ch != '\n') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am thinking if something like this could work:
char lastChar = ' ';
while (stream.good() && (lastChar == '\\' || (ch != '\r' && ch != '\n')) {
currentToken += ch;
lastChar = ch;
ch = stream.readChar();
}
@@ -434,7 +434,7 @@ static void comment_multiline() | |||
const char code[] = "#define ABC {// \\\n" | |||
"}\n" | |||
"void f() ABC\n"; | |||
ASSERT_EQUALS("\n\nvoid f ( ) { }", preprocess(code)); | |||
ASSERT_EQUALS("\n\nvoid f ( ) {", preprocess(code)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this the only line splicing test we have?
I think we should test what happens when there are multiple newlines after the :
// x \\\n
\n
\n
...
and an explicit test for \r\n:
// x \\\r\n
...
and I wonder what the behavior (according to standard / according to gcc+msvc+etc) is if there is space after the backslash:
// x \\ \n
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember exactly how the location is adjusted right now. Will the locations below the line splicing comment be correct? i.e.:
// hello \
world
x=1;
will x=1;
line number be 3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this the only line splicing test we have?
I think we should test what happens when there are multiple newlines after the :
// x \\\n \n \n ...
and an explicit test for \r\n:
// x \\\r\n ...
and I wonder what the behavior (according to standard / according to gcc+msvc+etc) is if there is space after the backslash:
// x \\ \n ...
Hi Danmar,
I wonder what the behavior (according to standard / according to gcc+msvc+etc) is if there is space after the backslash
I checked this with the gcc. The gcc version is 13.3.0.
If there is space after the backslash, the next line will be commented. That is different with the case there is a non-space char after the backslash.
#include <stdio.h>
int main() {
int x;
//abc \
printf("there is a space\n");
//abc \x
printf("there is a char x\n");
//abc \
printf("there is nothing\n");
return 0;
}
Output:
there is a char x
The 1st and 3rd output both didn't happen.
Interetingly, the editor seems have a different behavior with the gcc. I used the geditor on ubuntu. The editor will display the comment line with a color of purple. But it doesn't treat the first print as a comment line.
But obviously I think we should follow the rule of the gcc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember exactly how the location is adjusted right now. Will the locations below the line splicing comment be correct? i.e.:
// hello \ world x=1;
will
x=1;
line number be 3?
Hi Danmar,
I made a test for how we handle the comment marked by /**/.
readfile("abc/*123\\\n456\n789*/xxx")
output:
abc /*123456789*/ xxx
If there is a '\' in the comments, all the '\n' will be erased. As the multiline is not 0, location.adjust will not be called, and 'xxx' is not a newline, so the loc of 'xxx' will not be added with the multiline.
readfile("abc/*123\n456\n789*/xxx")
output:
abc /*123
456
789*/xxx
If there is only the '\n', multiline is not set. And the loc of the next token is adjusted. The loc will be added by one with '\r', '\n' or '\r\n' in the location.adjust.
For:
// hello \
world
x=1;
According to /**/, '\\n' or '\n' will add multiline by one. As 'x=1' is on a new line, it's line number of loc will be added with the multiline. So it will be 3, which is 'location.line += multiline + 1' ,and multiline = 1. I will update the commit according to the way of handling the multiline comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks! good that you tested this
tmp_ch = stream.readChar(); | ||
} | ||
if(!stream.good()) { | ||
if(!tmp.empty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this if (!tmp.empty())
is redundant.
} | ||
|
||
ch = tmp_ch; | ||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this continue is not need right?
tmp_ch = stream.readChar(); | ||
} | ||
if(!stream.good()) { | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this break can be removed right since the outer loop will break if stream is not good.
To fix the issue:
#337