You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by "Jaeyoung22 (via GitHub)" <gi...@apache.org> on 2023/09/05 22:50:33 UTC
[GitHub] [tomcat] Jaeyoung22 opened a new pull request, #661: Unify conditional statement format for some constants in http1 package
Jaeyoung22 opened a new pull request, #661:
URL: https://github.com/apache/tomcat/pull/661
Although I totally agree that there could be some unnecessary parentheses to aid the readability of the code, I think that the unity of the code is still important for readability.
### Conditional statements of CR & LF in Http11InputBuffer
```java
// Line 445
if (prevChr == Constants.CR && chr != Constants.LF) {
// ...
}
// Line 544
} else if (prevChr == Constants.CR && chr == Constants.LF) {
// ...
}
```
Except for Line 372 that I changed, all conditional statements about CR and LF don't have parentheses for each condition.
### Conditional statements in ChunkedInputFilter
```java
// Line 344
if (chr == Constants.CR || chr == Constants.LF) {
// ...
}
// Line 462
if (chr == Constants.CR || chr == Constants.LF) {
// ...
}
// Line 551
if (chr == Constants.CR || chr == Constants.LF) {
// ...
}
```
Except for Line 487 and 525 that I changed, all conditional statements about constants don't have parentheses for each condition.
### Logic about space and tab in Http11InputBuffer
```java
// Line 395, 453, 974(else if)
if (chr == Constants.SP || chr == Constants.HT) {
// ...
}
// Line 419, 515, 935
if (!(chr == Constants.SP || chr == Constants.HT)) {
// ...
}
```
Because of the above codes, I tried to change Line 1001 of Http11InputBuffer, and Line 577 of ChunkedInputFilter.
--
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@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] Jaeyoung22 commented on pull request #661: Unify conditional statement format for some constants in http1 package
Posted by "Jaeyoung22 (via GitHub)" <gi...@apache.org>.
Jaeyoung22 commented on PR #661:
URL: https://github.com/apache/tomcat/pull/661#issuecomment-1708220046
I've taken note of what you've said.
## 1. remove unnecessary parentheses throughout the package
Personally, I find conditional statements with three or more conditions to be more readable with parentheses, so I changed all conditional statements with TWO conditions where each condition is wrapped in parentheses.
## 2. Simplify conditional expressions using DeMorgan's law
As you mentioned, I recognized that not using DeMorgan's rule can be more readable, so I changed all conditional statements that use DeMorgan's rule to not use it.
Please let me know if there are any conditional statements that you think would be better readable if they used DeMorgan's law.
## Comment
This is my first PR contributing to open source, so there may be some imperfections. If I made a mistake or did something wrong, please forgive me and let me know. Thank you for your review!
--
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@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on pull request #661: Unify conditional statement format
Posted by "markt-asf (via GitHub)" <gi...@apache.org>.
markt-asf commented on PR #661:
URL: https://github.com/apache/tomcat/pull/661#issuecomment-1708336648
Thanks for the PR. It is so much more efficient to review this sort of change at this sort of scale.
Thanks for the De Morgan's law changes. I think those conditions are a lot easier to follow now.
I'm not sure if some of the Panama code is generated or not but I don't think applying this change will hurt even if it is.
--
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@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on a diff in pull request #661: Unify conditional statement format for some constants in http1 package
Posted by "markt-asf (via GitHub)" <gi...@apache.org>.
markt-asf commented on code in PR #661:
URL: https://github.com/apache/tomcat/pull/661#discussion_r1316875467
##########
java/org/apache/coyote/http11/filters/ChunkedInputFilter.java:
##########
@@ -574,7 +574,7 @@ private boolean parseHeader() throws IOException {
}
chr = readChunk.get(readChunk.position());
- if ((chr != Constants.SP) && (chr != Constants.HT)) {
+ if (!(chr == Constants.SP || chr == Constants.HT)) {
Review Comment:
As above - remove application of De Morgan's law.
##########
java/org/apache/coyote/http11/Http11InputBuffer.java:
##########
@@ -998,7 +998,7 @@ private HeaderParseStatus parseHeader() throws IOException {
byte peek = byteBuffer.get(byteBuffer.position());
if (headerParsePos == HeaderParsePosition.HEADER_MULTI_LINE) {
- if ((peek != Constants.SP) && (peek != Constants.HT)) {
+ if (!(peek == Constants.SP || peek == Constants.HT)) {
Review Comment:
Removing the unnecessary brackets is fine. Applying De Morgan's law is not. Yes, the result is the same but the intention is less obvious than with the original form so please keep 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@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on pull request #661: Unify conditional statement format for some constants in http1 package
Posted by "markt-asf (via GitHub)" <gi...@apache.org>.
markt-asf commented on PR #661:
URL: https://github.com/apache/tomcat/pull/661#issuecomment-1707956244
See #663 - a single PR would be preferred.
--
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@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] markt-asf merged pull request #661: Unify conditional statement format
Posted by "markt-asf (via GitHub)" <gi...@apache.org>.
markt-asf merged PR #661:
URL: https://github.com/apache/tomcat/pull/661
--
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@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org