You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@felix.apache.org by "DominikSuess (via GitHub)" <gi...@apache.org> on 2023/11/28 10:27:42 UTC
[PR] FELIX-6672 - rewrite of detection logic to save previous char, resolv… [felix-dev]
DominikSuess opened a new pull request, #250:
URL: https://github.com/apache/felix-dev/pull/250
…ing patterns (esp) start/stop tokens (//, /*, */) spreading across two reads.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FELIX-6672 - rewrite of detection logic to save previous char, resolv… [felix-dev]
Posted by "rombert (via GitHub)" <gi...@apache.org>.
rombert merged PR #250:
URL: https://github.com/apache/felix-dev/pull/250
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FELIX-6672 - rewrite of detection logic to save previous char, resolv… [felix-dev]
Posted by "jsedding (via GitHub)" <gi...@apache.org>.
jsedding commented on code in PR #250:
URL: https://github.com/apache/felix-dev/pull/250#discussion_r1412885818
##########
cm.json/src/main/java/org/apache/felix/cm/json/io/impl/JsonSupport.java:
##########
@@ -268,84 +270,109 @@ public int read(char[] cbuf, int off, int len) throws IOException {
// Detect String start/end if not inside a comment
if (!insideComment && !insideLineComment) {
if (c == '"') {
- // handle escaped quotes inside strings
- if (i > 0 && cbuf[i - 1] == '\\') {
- // escaped quote inside a string, include both the backslash and the quote
- currentLine.append(c);
- continue;
+ // only flip if not escaped quotes
+ if (oldChar != '\\') {
+ insideString = !insideString;
}
- insideString = !insideString;
+ currentLine.append(c);
+ oldChar = c;
+ continue;
}
}
// Handle comments only if not inside a string
if (!insideString) {
- // Detect start of single-line comment
- if (!insideComment && c == '/' && i < off + charsRead - 1 && cbuf[i + 1] == '/') {
- insideLineComment = true;
- i++; // Skip '/' character
+ // Detect potential start of a comment by detecting a slash
+ if(!insideComment && !insideLineComment && c == '/') {
+ // If the previous character was also a slash, we are inside a single-line comment
+ if (oldChar == '/') {
+ insideLineComment = true;
+ isSkippedSlash = false;
+ } else {
+ // skipping slash for verification if this is comment - will be ammended on next char if non-comment
+ isSkippedSlash = true;
+ }
+ oldChar = c;
continue;
}
-
- // Detect start of multi-line comment
- if (!insideComment && c == '/' && i < off + charsRead - 1 && cbuf[i + 1] == '*') {
- insideComment = true;
- i++; // Skip '*' character
+ // Detect potential start of a multiline comment by detecting a star
+ if(!insideComment && !insideLineComment && c == '*') {
+ // If the previous character was also a slash, we are inside a multi-line comment
+ if (oldChar == '/') {
+ insideComment = true;
+ isSkippedSlash = false;
+ } else {
+ // otherwise this is not a comment, just a star
+ currentLine.append(c);
+ }
+ oldChar = c;
continue;
}
- // Detect end of multi-line comment
- if (insideComment && c == '*' && i < off + charsRead - 1 && cbuf[i + 1] == '/') {
- insideComment = false;
- i++; // Skip '/' character
- continue;
+ // if if we skipped over a / above and we're not within a comment, we need to append the oldChar to the currentLine
+ if (!insideComment && !insideLineComment && isSkippedSlash) {
+ currentLine.append('/');
+ isSkippedSlash = false;
}
- // Skip characters inside single-line comments
+ // Detect potential end of a linecomment by detecting a newline
if (insideLineComment && c == '\n') {
insideLineComment = false;
+ currentLine.append(c);
+ oldChar = c;
+ continue;
}
- // Preserve newline characters inside multiline comments
- if (insideComment && !insideLineComment && c == '\n') {
- currentLine.append(c);
+ // Skip over characters inside single-line comments
+ if (insideLineComment) {
+ oldChar = c;
continue;
}
- // Skip characters inside multiline comments
- if (insideComment && c == '*' && i < off + charsRead - 1 && cbuf[i + 1] == '/') {
+ // Detect potential end of a multiline comment by detecting a slash that is preceded by a star
+ if (insideComment && c == '/' && oldChar == '*') {
insideComment = false;
- i++; // Skip '/' character
+ oldChar = c;
continue;
}
- // Skip characters inside single-line comments
- if (insideLineComment && c == '\n') {
- insideLineComment = false;
+ // Skip over characters inside multi-line comments but preserve newlines
+ if (insideComment) {
+ if(c == '\n') {
+ currentLine.append(c);
+ }
+ oldChar = c;
+ continue;
}
}
-
- // Preserve characters outside comments
- if (!insideComment && !insideLineComment) {
- currentLine.append(c);
- }
+ // Preserve characters outside comments
+ if (!insideComment && !insideLineComment) {
+ currentLine.append(c);
}
+ oldChar = c;
+ }
- filteredContent.append(currentLine.toString());
+ filteredContent.append(currentLine.toString());
- char[] filteredChars = filteredContent.toString().toCharArray();
- int filteredLen = Math.min(filteredChars.length, len);
- System.arraycopy(filteredChars, 0, cbuf, off, filteredLen);
- return filteredLen;
- }
- return charsRead;
+ char[] filteredChars = filteredContent.toString().toCharArray();
+ int filteredLen = Math.min(filteredChars.length, len);
Review Comment:
Wouldn't it be a bug if `filteredChars.length > len`? If so, maybe throwing an exception would be better than smoothing over it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FELIX-6672 - rewrite of detection logic to save previous char, resolv… [felix-dev]
Posted by "jsedding (via GitHub)" <gi...@apache.org>.
jsedding commented on code in PR #250:
URL: https://github.com/apache/felix-dev/pull/250#discussion_r1412885508
##########
cm.json/src/test/java/org/apache/felix/cm/json/io/impl/JsonSupportTest.java:
##########
@@ -307,4 +307,19 @@ public void testEscapedQuotes() throws IOException {
+ "}\n", parse(input));
}
+ @Test
+ public void testSlashAtEndOfRead() throws Exception {
+ String input = "This is a test string // Next string \n"
+ + "next line" ;
+ Reader reader = JsonSupport.createCommentRemovingReader(new StringReader(input));
+
+ char[] buffer = new char[22];
Review Comment:
Nit: Instead of the magic number 22, how about using `input.indexOf('/')`? Or something equivalent, if I misunderstood the intent. IMHO that would make it clearer what you're testing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FELIX-6672 - rewrite of detection logic to save previous char, resolv… [felix-dev]
Posted by "jsedding (via GitHub)" <gi...@apache.org>.
jsedding commented on PR #250:
URL: https://github.com/apache/felix-dev/pull/250#issuecomment-1837267514
Do you consider the case, where the first read reads e.g. `"Foo bar /* a block comment \n on two lines *"` and the second read reads `"/ more stuff after the comment"`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org