[PATCH] D111273: [clang-format-diff] Fix missing formatting for zero length git diff lines

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay accepted this revision.
MyDeveloperDay added a comment.

So this isn't just clang-format-diff.py but also all of `git clang-format`

  <edit the file>
  git add Analysis.cpp
  $ git clang-format
  clang-format did not modify any files

This is bad because this is a way in which those using "git clang-format" can miss formatting a line

So ultimately looking at the diff (normal diff)

  $ git diff --cached Analysis.cpp
  diff --git a/llvm/lib/CodeGen/Analysis.cpp b/llvm/lib/CodeGen/Analysis.cpp
  index e5d576d879b5..3107fc0b9936 100644
  --- a/llvm/lib/CodeGen/Analysis.cpp
  +++ b/llvm/lib/CodeGen/Analysis.cpp
  @@ -115,7 +115,6 @@ void llvm::ComputeValueVTs(const TargetLowering &TLI, const DataLayout &DL,
       return;
     // Base case: we can get an EVT for this LLVM IR type.
     ValueVTs.push_back(TLI.getValueType(DL, Ty));
  -  if (MemVTs)
       MemVTs->push_back(TLI.getMemValueType(DL, Ty));
     if (Offsets)
       Offsets->push_back(StartingOffset);

This is going to give us a start_line of 115 and a line_count of 6 and hence an end_line of 115+5 = 120

F19483241: image.png <https://reviews.llvm.org/F19483241>

if clang-format is given those lines I WOULD expect it to continue and correct it.

  $ clang-format -n --lines 115:120 Analysis.cpp
  Analysis.cpp:117:48: warning: code should be clang-formatted [-Wclang-format-violations]
    ValueVTs.push_back(TLI.getValueType(DL, Ty));

But like clang-format-diff here, git-clang-format is doing a -U0 diff hence the line count is 0

  $ git diff -U0 --cached Analysis.cpp
  diff --git a/llvm/lib/CodeGen/Analysis.cpp b/llvm/lib/CodeGen/Analysis.cpp
  index e5d576d879b5..3107fc0b9936 100644
  --- a/llvm/lib/CodeGen/Analysis.cpp
  +++ b/llvm/lib/CodeGen/Analysis.cpp
  @@ -118 +117,0 @@ void llvm::ComputeValueVTs(const TargetLowering &TLI, const DataLayout &DL,
  -  if (MemVTs)

git-clang-format would also not process that change (as line_count == 0)

The code for handling this in git-clang-format is subtly different but equally ootentially wrong

  def extract_lines(patch_file):
    """Extract the changed lines in `patch_file`.
  
    The return value is a dictionary mapping filename to a list of (start_line,
    line_count) pairs.
  
    The input must have been produced with ``-U0``, meaning unidiff format with
    zero lines of context.  The return value is a dict mapping filename to a
    list of line `Range`s."""
    matches = {}
    for line in patch_file:
      line = convert_string(line)
      match = re.search(r'^\+\+\+\ [^/]+/(.*)', line)
      if match:
        filename = match.group(1).rstrip('\r\n')
      match = re.search(r'^@@ -[0-9,]+ \+(\d+)(,(\d+))?', line)
      if match:
        start_line = int(match.group(1))
        line_count = 1
        if match.group(3):
          line_count = int(match.group(3))
        if line_count > 0:
          matches.setdefault(filename, []).append(Range(start_line, line_count))
    return matches

However if we take the same approach as suggested here, clang-format will run over that line

  $ clang-format -n --lines 117:117 Analysis.cpp
  Analysis.cpp:117:48: warning: code should be clang-formatted [-Wclang-format-violations]
    ValueVTs.push_back(TLI.getValueType(DL, Ty));

To answer @hans question, I think it will do the correct thing, clang-format still works on the whole file, it just filters the replacements based on the "line range" given by --lines so I would expect it to work

I think this is the correct change being proposed here, but I also think git-clang-format is wrong.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111273