[PATCH] D68682: Clang-tidy fix removals removing all non-blank text from a line should remove the line

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc updated this revision to Diff 224089.
poelmanc retitled this revision from "Clang-tidy fixes should avoid creating new blank lines" to "Clang-tidy fix removals removing all non-blank text from a line should remove the line".
poelmanc edited the summary of this revision.
poelmanc added a comment.

Thanks for the prompt review and feedback! MyDeveloperDay, your "drive-by" comments are quite welcome as the goal is to make clang-tidy and clang-format work better in tandem. You are correct that my prior version could have thwarted someone's chang-tidy checker that adds whitespace, which was an oversight on my part. I've updated the patch to change `isWhitespace( RemovalText )` to `RemovalText.isBlank()`. Now this only applies to //Removals// that remove all non-blank text from a line. I also updated the Title and Summary accordingly, adding the example that most confounded me of ": a()\n", which readability-redundant-member-init makes blank with a combination of two separate removals: one for the ":" and one for the "a()".

I understand and appreciate the feedback:

- //this removal of extra white space needs to be handled at the point the replacement is created and not on all the final replacements where the context is lost//
- //Checks that edit the source code should be improved to delete newlines where they become unnecessary//

Indeed I first tried fixing this entirely within readability-redundant-member-init. One problem was that when a line becomes blank due to two separate removals, code that peeks ahead or backwards to see if the line is blank doesn't know about other removals that will occur. Perhaps this can be fixed if the check() function has access to other removals that have already been created? (See also //Note 1//.) As I broadened my search I found code in readability-redundant-control-flow that heuristically tried to remove blank lines, but it failed to examine the entire line and thus removed newlines from the ends of lines that were not actually blank. (See the test case I added to readability-redundant-control-flow.cpp, where `  /* NOTE */ continue;\n` had the newline following `continue;` removed, leaving the comment attached to the next line.) And I found that readability-redundant-declaration did not attempt to remove blank lines, and its test cases specifically added comments to all removed declarations so that in its test cases, none of the resulting lines were blank. (See the test case I added to readability-redundant-declaration.cpp.)

Digging into these examples led me to the exact opposite conclusion: that the "newly blank line" problem needs to be solved consistently at a higher-level where all removals can be examined jointly, rather than trying to address it within each check.

I guess here's the high-level question: //should all removals that remove all non-blank text from a line also delete the line//?

- If //yes//: consider reviewing the patch
- If //mostly//: consider adding this as a top-level clang-tidy option that's off by default
- //Otherwise//: if it's possible to address this solely within each of readability-redundant-member-init, readability-redundant-control-flow, readability-redundant-declaration, etc., I can revisit that original approach. I'd appreciate pointers on how to access prior Removals on the same line from within check(), and any guesses regarding the Windows line-ending issue of //Note 1//.

Thanks!

//Note 1:// When applied to a file with Windows-style 2-character line endings, adding a Removal to readability-redundant-member-init whose Length was set to include the two-character newline would delete only the first character of the newline, leaving a \r. Then when I increased the removal length by 1, it would delete both newline characters //and// the first character of the line //after// the newline, which would be extremely undesirable. Updating removals at the top-level of ClangTidy.cpp did not have this problem. I was never able to track down where/why this error occurred, though it's surely fixable.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68682/new/

https://reviews.llvm.org/D68682

Files:
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.cpp
  clang-tools-extra/test/clang-tidy/readability-redundant-control-flow.cpp
  clang-tools-extra/test/clang-tidy/readability-redundant-declaration.cpp
  clang-tools-extra/test/clang-tidy/readability-redundant-member-init.cpp
  clang/include/clang/AST/CommentLexer.h
  clang/include/clang/AST/CommentParser.h
  clang/lib/AST/CommentLexer.cpp
  clang/lib/AST/CommentParser.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D68682.224089.patch
Type: text/x-patch
Size: 12252 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20191009/06b4e03a/attachment-0001.bin>