Conversation
ruanwenjun
force-pushed
the
dev_wenjun_removeUnusedComment
branch
from
68f3b5d to
9015b2e
Compare
| } | ||
| for (final Logger child : childrenList) { | ||
| if (childName.equals(child.getName())) { | ||
| return child; | ||
| } |
There was a problem hiding this comment.
I like this optimization, but I propose taking it further and ensuring that childrenList is always an instance of CopyOnWriteArrayList
transient private List<Logger> childrenList;↓↓↓
transient private final List<Logger> childrenList = new CopyOnWriteArrayList<>();or, with modifiers sorted
private final transient List<Logger> childrenList = new CopyOnWriteArrayList<>();Then all the null checks could be removed and there would be not even a remote possibility that childrenList's iterator could throw ConcurrentModificationException.
It may even fix some subtle visibility issues. There are some unsynchronized null checks against childrenList that may read a stale value. e.g.
Logger createChildByLastNamePart(final String lastPart) {
…
if (childrenList == null) {
^^^^^^^^^^^^^^^^^^^^
childrenList = new CopyOnWriteArrayList<Logger>();
}
…
}There was a problem hiding this comment.
Very good suggestion, I can't agree more, updated.
| for (int i = 0; i < len; i++) { | ||
| Logger child = (Logger) childrenList.get(i); | ||
| for (Logger childLogger : childrenList) { | ||
| // tell child to handle parent levelInt change | ||
| child.handleParentLevelChange(effectiveLevelInt); | ||
| childLogger.handleParentLevelChange(effectiveLevelInt); |
| for (Logger childLogger : childrenList) { | ||
| childLogger.handleParentLevelChange(newParentLevelInt); |
| /** | ||
| * The default size of child list arrays. The JDK 1.5 default is 10. We use a | ||
| * smaller value to save a little space. | ||
| */ |
| * | ||
| * |
There was a problem hiding this comment.
I'd be inclined to take out unnecessary, unrelated changes to make it easier on the maintainers
There was a problem hiding this comment.
Sorry, this is caused by my IDEA setting, now I revert this.
ruanwenjun
force-pushed
the
dev_wenjun_removeUnusedComment
branch
2 times, most recently
from
4b8167c to
03cd226
Compare
mches
left a comment
mches
left a comment
There was a problem hiding this comment.
Your PR title is "Remove Unused comment in LoggerContext", but the comment you removed is in Logger. In LoggerContext you removed redundant casting and type information and unused imports. These codebase modernization activities could be done en masse using tools, but don't seem to be a project priority. I'd be inclined to separate or discard inconsequential changes. In Logger you do many things other than remove a comment. I suggest focusing your PR and not neglecting the PR title and description. Maintainers appreciate an accurate title and overview.
Thanks suggestions, this help me not only on this PR but in the future work. So I need to separate this PR and change the title to /**
* The default size of child list arrays. The JDK 1.5 default is 10. We use a
* smaller value to save a little space.
*/ |
|
No guarantee to get attention of maintainers, but that sounds like a focused PR to me. I encourage you to read the contribution guidelines. Wish you luck. |
Signed-off-by: Wenjun Ruan <wenjun@apache.org>
ruanwenjun
force-pushed
the
dev_wenjun_removeUnusedComment
branch
from
03cd226 to
7419ed9
Compare
ruanwenjun
changed the title
2 participants
No description provided.