[webkit-reviews] review denied: [Bug 27610] Add check for line-breaking rule #3 to cpplint : [Attachment 33347] Add check for line-breaking rule #3 to cpplint

[bugzilla-daemon at webkit.org]

Adam Treat <treat at kde.org> has denied Jakob Petsovits <jpetso at gmx.at>'s request
for review:
Bug 27610: Add check for line-breaking rule #3 to cpplint
https://bugs.webkit.org/show_bug.cgi?id=27610

Attachment 33347: Add check for line-breaking rule #3 to cpplint
https://bugs.webkit.org/attachment.cgi?id=33347&action=review

------- Additional Comments from Adam Treat <treat at kde.org>
First, I want to say that this is awesome.  I wasn't sure we'd be able to
easily check this style rule.

> +    readability/else_if

So you are adding an entirely new category.  Far enough.  At this point I want
to say I think this test should check for the following situation too:

if (true) {
    foo();
    return;
} else {
    bar();
}

The style guide doesn't explicitly say it, but I think it is implied.  Why on
earth would we allow the above, but deny for 'else if' ?

As such, I think the name of the category as well as the rest of the code
should be changed to reflect this case as well and test cases added.

The style guide should probably be updated to make it explicit as well.

> +def check_else_if_with_prior_return(filename, clean_lines, line_number,
error):

See above re: naming of method.

> +    elseif_match =
match(r'(?P<elseif_indentation>\s*)(?P<brace>\}\s*)?else\s+if\s*\(', line)
> +    if not elseif_match:
> +	   return
> +
> +    elseif_indentation = elseif_match.group('elseif_indentation')
> +    inner_indentation = elseif_indentation + ' ' * 4
> +    expect_closing_brace = elseif_match.start('brace') == -1

Ok, I follow.  You are going to rely upon indentation to tell the story.  This
is going to produce false positives for places where the code is improperly
indented and it is going to miss
a lot of cases too potentially.  That is ok, though as we understand cpplint
has false positives and is only meant as a guide in addition to review.

> +	   # Skip not only empty lines but also those with preprocessor
directives
> +	   # and goto labels.
> +	   if current_line.strip() == '' or current_line.startswith('#') or
match(r'\w+\s*:\s*$', current_line):
> +	       continue

Perhaps it'd be good to break these regex's out where they are used in multiple
places by cpplint.  RE_PATTERN_INCLUDE does this with good success.

Maybe RE_PATTERN_GOTO and RE_PATTERN_PREPROCESSOR too?	Good refactoring
possibility to think about.

> +	   # Skip lines with closing braces on the original indentation level.
> +	   # Even though the styleguide says they should be on the same line as

> +	   # the "else if" statement, we also want to check for instances where

> +	   # the current code does not comply with the coding style. Thus,
ignore
> +	   # these lines and proceed to the line before that.
> +	   if expect_closing_brace and current_line == elseif_indentation +
'}':
> +	       expect_closing_brace = False
> +	       continue

After this you don't expect a closing brace, right?  Where is
'expect_closing_brace' used again?  I don't see it...  Is this variable
necessary?  What happens when you expect a closing brace and it never comes
because the prior if doesn't have braces?

> +	   # As we're going up the lines, the first real statement to encounter

> +	   # has to be a return statement - otherwise, this check doesn't
apply.
> +	   if not encountered_return:
> +	       if current_line.startswith(inner_indentation + 'return ') or
current_line.startswith(inner_indentation + 'return;'):
> +		   encountered_return = True
> +		   continue
> +	       else:
> +		   break

Why not just, "if current_line.startswith(inner_indentation + 'return')" ?

> +	   # When code execution reaches this point, we have found a "return"
line
> +	   # as last statement of the previous block. Now we only need to make
> +	   # sure that the block belongs to an "if", and we can throw an error.


What else is it going to belong to? an 'else if'? if it is an 'else if' 
shouldn't we complain for that too?

    if (condition1) {
	foo1();
    } else if (condition2) {
	foo2();
	return;
    } else if (condition3 {
	foo3();
    } else {
	foo4();
    }

That should be rewritten too, no?

    if (!condition1 && condition2) {
	foo2();
	return;
    }

    if (condition1) {
	foo1();
    } else if (condition3 {
	foo3();
    } else {
	foo4();
    }

What do others think?

> +	   # Skip lines with opening braces on the original indentation level,
> +	   # similar to the closing braces check above.
> +	   if current_line == elseif_indentation + '{':
> +	       continue

I don't follow this...

> +	   current_indentation_match =
match(r'(?P<indentation>\s*)(?P<remaining_line>.*)$', current_line);
> +	   current_indentation = current_indentation_match.group('indentation')

> +	   remaining_line = current_indentation_match.group('remaining_line')

Guess you are looking for original 'if'...

> +	   # Skip everything that's further indented than our "else if".
> +	   if current_indentation.startswith(elseif_indentation) and
current_indentation != elseif_indentation:
> +	       continue
> +
> +	   # So we've got a line with same (or less) indentation. Is it an
"if"?
> +	   # If yes: throw an error. If no: don't throw an error.
> +	   # Whatever the outcome, this is the end of our loop.
> +	   if match(r'if\s*\(', remaining_line):
> +	       error(filename, line_number + line_offset,
'readability/else_if', 4,
> +		     'An else if statement should be written as an if statement
when the prior if concludes with a return statement.')
> +	   break

Can be greatly simplified if we also consider above I think.  Then you won't
have to go searching for original if.

I'd like to get some others feedback if possible.