Issue 851

[R Veach]

Issue: https://github.com/checkstyle/checkstyle/issues/851
Code change: https://github.com/rnveach/checkstyle/commit/a5bf98b0a2c0e049daece2276c7bb5a0bfdb1115

I made changes to pull in the comment if it is next after the end of the current tree branch, but it is making more changes to the tree than I originally anticipated. I am posting to confirm here that these changes are ok, and if not, and how to go about fixing it.

Example:
Input: https://github.com/rnveach/checkstyle/blob/master/src/test/resources/com/puppycrawl/tools/checkstyle/comments/InputFullOfBlockComments.java
Before Changes Tree: http://pastebin.com/AeXfkLk1
After Changes Tree: http://pastebin.com/dKqWFyMR

Let me know if there is a better way to provide a view of this. I found http://www.quickdiff.com/ to have the better online compare.

Summary of changes:

Major Changes:
These are changes where comments moved drastically looking at a straight up/down view.

Comments 12,13,14 are now siblings to line 1's PACKAGE_DEF > SEMI instead of line 3's CLASS_DEF > MODIFIER.

Pretty view: http://pastebin.com/ETrWsZme

Comment 21 is now a sibling to line 5's CLASS_DEF > OBJBLOCK > METHOD_DEF > LITERAL_STATIC instead of line 7's CLASS_DEF > OBJBLOCK > METHOD_DEF > TYPE > IDENT.

Pretty view: http://pastebin.com/4sW74trT

Minor Changes:
These are changes where the comments moved to their previous siblings, last child node. They are still in the same placement from a straight up/down view.

Comments 11,15,22,27,28,32,35,44,45,53,54,56,60, and 47.

All other comments remained the same.

[Roman Ivanov]

HI R Veach,

Sorry for delay to answer you.

Thank you very much for detailed analysis of the problem.

1) 

It would be very cool if you put to this post AST tree structure for a case that described in github issue. But I presume that comment is appeared as child of second CASE.

2)

> Pretty view: http://pastebin.com/ETrWsZme

as you can see previously we had comments before class be a part/child of CLASS node.

Real example: javadoc is part/child of CLASS.

Text "CLASS_DEF" (CLASS_DEF) Line 3 Column 6
	Text "MODIFIERS" (MODIFIERS) Line 3 Column 6
                Text "/*" (BLOCK_COMMENT_BEGIN) Line 1 Column 49
                        Text "12" (COMMENT_CONTENT) Line 1 Column 51
                        Text "*/" (BLOCK_COMMENT_END) Line 1 Column 52
                Text "/*" (BLOCK_COMMENT_BEGIN) Line 2 Column 0
                        Text "13" (COMMENT_CONTENT) Line 2 Column 2
                        Text "*/" (BLOCK_COMMENT_END) Line 2 Column 3
                Text "/*" (BLOCK_COMMENT_BEGIN) Line 3 Column 0
                        Text "14" (COMMENT_CONTENT) Line 3 Column 2
                        Text "*/" (BLOCK_COMMENT_END) Line 3 Column 3
		Text "public" (LITERAL_PUBLIC) Line 3 Column 6

This is may be not ideal but reasonable form.

But after update it become part/child of PACKAGE

Text "package" (PACKAGE_DEF) Line 1 Column 0
	Text "ANNOTATIONS" (ANNOTATIONS) Line 1 Column 39
	Text "." (DOT) Line 1 Column 39
		Text "." (DOT) Line 1 Column 28
			Text "." (DOT) Line 1 Column 22
				Text "." (DOT) Line 1 Column 11
					Text "com" (IDENT) Line 1 Column 8
					Text "puppycrawl" (IDENT) Line 1 Column 12
				Text "tools" (IDENT) Line 1 Column 23
			Text "checkstyle" (IDENT) Line 1 Column 29
		Text "comments" (IDENT) Line 1 Column 40
	Text ";" (SEMI) Line 1 Column 48
	Text "/*" (BLOCK_COMMENT_BEGIN) Line 1 Column 49
		Text "12" (COMMENT_CONTENT) Line 1 Column 51
		Text "*/" (BLOCK_COMMENT_END) Line 1 Column 52
	Text "/*" (BLOCK_COMMENT_BEGIN) Line 2 Column 0
		Text "13" (COMMENT_CONTENT) Line 2 Column 2
		Text "*/" (BLOCK_COMMENT_END) Line 2 Column 3
	Text "/*" (BLOCK_COMMENT_BEGIN) Line 3 Column 0
		Text "14" (COMMENT_CONTENT) Line 3 Column 2
		Text "*/" (BLOCK_COMMENT_END) Line 3 Column 3
Text "CLASS_DEF" (CLASS_DEF) Line 3 Column 6

this structure I can not explain at all.

So this update is not acceptable and weird.

------------------------------------------------------------------

3)

> Pretty view: http://pastebin.com/4sW74trT

This update is ok (it is not better or worse then before). Comment was a description of TYPE (so was a child of TYPE) that is the same as javadoc of next element. After update it becomes sibling of all other modifiers  that is also we could explain and reasonable.

Text "CLASS_DEF" (CLASS_DEF) Line 3 Column 0
	Text "MODIFIERS" (MODIFIERS) Line 3 Column 0
		Text "public" (LITERAL_PUBLIC) Line 3 Column 0
	Text "class" (LITERAL_CLASS) Line 3 Column 7
	Text "InputFullOfBlockComments" (IDENT) Line 3 Column 13
	Text "OBJBLOCK" (OBJBLOCK) Line 3 Column 38
		Text "{" (LCURLY) Line 3 Column 38
		Text "METHOD_DEF" (METHOD_DEF) Line 4 Column 1
			Text "MODIFIERS" (MODIFIERS) Line 4 Column 1
				Text "public" (LITERAL_PUBLIC) Line 4 Column 1
				Text "static" (LITERAL_STATIC) Line 4 Column 8
			Text "TYPE" (TYPE) Line 6 Column 3
                                Text "/*" (BLOCK_COMMENT_BEGIN) Line 4 Column 14
                                        Text "21" (COMMENT_CONTENT) Line 4 Column 16
                                        Text "*/" (BLOCK_COMMENT_END) Line 6 Column 0
				Text "String" (IDENT) Line 6 Column 3

after update:

Text "CLASS_DEF" (CLASS_DEF) Line 3 Column 0
	Text "MODIFIERS" (MODIFIERS) Line 3 Column 0
		Text "public" (LITERAL_PUBLIC) Line 3 Column 0
	Text "class" (LITERAL_CLASS) Line 3 Column 7
	Text "InputFullOfBlockComments" (IDENT) Line 3 Column 13
	Text "OBJBLOCK" (OBJBLOCK) Line 3 Column 38
		Text "{" (LCURLY) Line 3 Column 38
		Text "METHOD_DEF" (METHOD_DEF) Line 4 Column 1
			Text "MODIFIERS" (MODIFIERS) Line 4 Column 1
				Text "public" (LITERAL_PUBLIC) Line 4 Column 1
				Text "static" (LITERAL_STATIC) Line 4 Column 8
				Text "/*" (BLOCK_COMMENT_BEGIN) Line 4 Column 14
					Text "21" (COMMENT_CONTENT) Line 4 Column 16
					Text "*/" (BLOCK_COMMENT_END) Line 6 Column 0
			Text "TYPE" (TYPE) Line 6 Column 3
				Text "String" (IDENT) Line 6 Column 3

4) I did not review all other cases as case "2)" is critical.

5) Before any other updates lets think one more time about location of comment.

It might become that only SWITCH-CASE is exceptional case when user by tail comment want to tell smth to others. 

What we will do if user try to describe CASE and by indentation put it above a CASE. Example:

// multiline tokens

case TokenTypes.WHILE:

case TokenTypes.IF:

case TokenTypes.ELSE: 

     doValidationOnMultiLine();

     break;

// single line tokens

case: TokenTypes.PACKAGE;

case: TokenTypes.STATEMENT;

    doSinleLineValidation();

   break;

in this case user is right too, and he does not want comment to be intended as a CASE.

So we can not automatically make a decision on a indentation of comment in switch-case block. So it is ok to have special logic at Checks to grab previous statement in a unusual way (this is how we finally did this in CommentIndentationCheck). 

CASE comments serve only one purpose - "// fall through", it might be reasonable to check content(by user defined template) of comment to make a decision of its location.

--------------------------------

After your details and my extra investigation I do not see a reason why we should change AST structure. SWITCH-CASE fall through comment is just a an exception from general model (so it is a problem for CommentIndentationCheck and this Check should handle this).

Andrew Selkin, please join to this discussion, you are an author of CommentIndentationCheck you might have more experience of current comments AST usage. Please share your opinion.

thanks,

Roman Ivanov

​

[Roman Ivanov]

HI RVeach and Andrew,

might be related issues:

https://github.com/checkstyle/checkstyle/issues/950

https://github.com/checkstyle/checkstyle/issues/2077

https://github.com/checkstyle/checkstyle/issues/2078

thanks,

Roman Ivanov

​