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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

The following fix should resolve this

  $ git diff git-clang-format
  diff --git a/clang/tools/clang-format/git-clang-format 
b/clang/tools/clang-format/git-clang-format
  index 40e848d55a8c..c7e15eb7b483 100755
  --- a/clang/tools/clang-format/git-clang-format
  +++ b/clang/tools/clang-format/git-clang-format
  @@ -327,6 +327,8 @@ def extract_lines(patch_file):
   line_count = int(match.group(3))
 if line_count == 0:
   line_count = 1
  +  if start_line == 0:
  +continue
 matches.setdefault(filename, []).append(Range(start_line, line_count))
 return matches

$ ./clang/tools/clang-format/git-clang-format -v -v HEAD~1
Ignoring changes in the following files (wrong extension or symlink):

  clang/tools/clang-format/git-clang-format

Running clang-format on the following files:

  llvm/include/llvm/ExecutionEngine/JITLink/TableManager.h
  llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp

old tree: 7584627b571deb7ed22068eb6e0496b51d76b63f
new tree: e88c2fb484c4fd0d63e150fa8d666c794ffd18ce
changed files:

  llvm/include/llvm/ExecutionEngine/JITLink/TableManager.h


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111273

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

Just guessing but is this caused by the text at the bottom? There doesn’t seem 
to be a zero line change in the patch file

—-
2.30.1 (Apple Git-130)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111273

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Lang Hames via Phabricator via cfe-commits]

lhames added a comment.

Heads up -- I think I just hit an error due to this while formatting a local 
commit:

  % ./clang/tools/clang-format/git-clang-format HEAD~1  
 
  Assertion failed: (Line && Col && "Line and column should start from 1!"), 
function translateLineCol, file 
./llvm-project/clang/lib/Basic/SourceManager.cpp, line 1699.
  PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash 
backtrace.
  Stack dump:   
 
  0.  Program arguments: clang-format -lines=15:15 -lines=24:24 
-lines=46:47 -lines=73:74 -lines=82:82 -lines=117:117 -lines=128:128 
-lines=168:168 -lines=177:177 -lines=0:0 
llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp
  Stack dump without symbol names (ensure you have llvm-symbolizer in your PATH 
or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it):  
 
  0  clang-format 0x000101390dad 
llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) + 61
  1  clang-format 0x00010139135b 
PrintStackTraceSignalHandler(void*) + 27
  2  clang-format 0x00010138f00b llvm::sys::RunSignalHandlers() 
+ 123
  3  clang-format 0x000101393fb8 SignalHandler(int) + 232   
 
  4  libsystem_platform.dylib 0x7fff20639d7d _sigtramp + 29 
 
  5  libsystem_platform.dylib 00 _sigtramp + 
18446603339972764320
  6  libsystem_c.dylib0x7fff20549411 abort + 120

 
  7  libsystem_c.dylib0x7fff205487e8 err + 0
  8  clang-format 0x00010146e898 
clang::SourceManager::translateLineCol(clang::FileID, unsigned int, unsigned 
int) const + 136
  9  clang-format 0x0001011eb586 
clang::format::fillRanges(llvm::MemoryBuffer*, 
std::__1::vector >&) + 966
 
  10 clang-format 0x0001011dd718 
clang::format::format(llvm::StringRef) + 1064   
  11 clang-format 0x0001011dcc1b main + 955 

 
  12 libdyld.dylib0x7fff2060ff3d start + 1  
 
  13 libdyld.dylib0x000c start + 18446603339972935888   

 
  error: `clang-format -lines=15:15 -lines=24:24 -lines=46:47 -lines=73:74 
-lines=82:82 -lines=117:117 -lines=128:128 -lines=168:168 -lines=177:177 
-lines=0:0 llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp` failed

Reverting this patch fixed the error.

I am able to reliably reproduce this crash on Darwin by running:

  % git checkout fd26ca4e7515e7dd32ae02e777bd21693afc68ff
  % git am jitlink-table-manager-updates.patch
  % ./clang/tools/clang-format/git-clang-format HEAD~1

F19699034: jitlink-table-manager-updates.patch 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111273

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Zequan Wu via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGf93169226a29: [clang-format-diff] Fix missing formatting for 
zero length git diff lines (authored by zequanwu).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111273

Files:
  clang/tools/clang-format/clang-format-diff.py
  clang/tools/clang-format/git-clang-format


Index: clang/tools/clang-format/git-clang-format
===
--- clang/tools/clang-format/git-clang-format
+++ clang/tools/clang-format/git-clang-format
@@ -321,8 +321,9 @@
   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))
+  if line_count == 0:
+line_count = 1
+  matches.setdefault(filename, []).append(Range(start_line, line_count))
   return matches
 
 
Index: clang/tools/clang-format/clang-format-diff.py
===
--- clang/tools/clang-format/clang-format-diff.py
+++ clang/tools/clang-format/clang-format-diff.py
@@ -90,9 +90,11 @@
   line_count = 1
   if match.group(3):
 line_count = int(match.group(3))
-  if line_count == 0:
-continue
-  end_line = start_line + line_count - 1
+  # Also format lines range if line_count is 0 in case of deleting
+  # surrounding statements.
+  end_line = start_line
+  if line_count != 0:
+end_line += line_count - 1
   lines_by_file.setdefault(filename, []).extend(
   ['-lines', str(start_line) + ':' + str(end_line)])
 


Index: clang/tools/clang-format/git-clang-format
===
--- clang/tools/clang-format/git-clang-format
+++ clang/tools/clang-format/git-clang-format
@@ -321,8 +321,9 @@
   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))
+  if line_count == 0:
+line_count = 1
+  matches.setdefault(filename, []).append(Range(start_line, line_count))
   return matches
 
 
Index: clang/tools/clang-format/clang-format-diff.py
===
--- clang/tools/clang-format/clang-format-diff.py
+++ clang/tools/clang-format/clang-format-diff.py
@@ -90,9 +90,11 @@
   line_count = 1
   if match.group(3):
 line_count = int(match.group(3))
-  if line_count == 0:
-continue
-  end_line = start_line + line_count - 1
+  # Also format lines range if line_count is 0 in case of deleting
+  # surrounding statements.
+  end_line = start_line
+  if line_count != 0:
+end_line += line_count - 1
   lines_by_file.setdefault(filename, []).extend(
   ['-lines', str(start_line) + ':' + str(end_line)])
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Zequan Wu via Phabricator via cfe-commits]

zequanwu updated this revision to Diff 378291.
zequanwu added a comment.

Fix the same error in git-clang-format.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111273

Files:
  clang/tools/clang-format/clang-format-diff.py
  clang/tools/clang-format/git-clang-format


Index: clang/tools/clang-format/git-clang-format
===
--- clang/tools/clang-format/git-clang-format
+++ clang/tools/clang-format/git-clang-format
@@ -321,8 +321,9 @@
   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))
+  if line_count == 0:
+line_count = 1
+  matches.setdefault(filename, []).append(Range(start_line, line_count))
   return matches
 
 
Index: clang/tools/clang-format/clang-format-diff.py
===
--- clang/tools/clang-format/clang-format-diff.py
+++ clang/tools/clang-format/clang-format-diff.py
@@ -90,9 +90,11 @@
   line_count = 1
   if match.group(3):
 line_count = int(match.group(3))
-  if line_count == 0:
-continue
-  end_line = start_line + line_count - 1
+  # Also format lines range if line_count is 0 in case of deleting
+  # surrounding statements.
+  end_line = start_line
+  if line_count != 0:
+end_line += line_count - 1
   lines_by_file.setdefault(filename, []).extend(
   ['-lines', str(start_line) + ':' + str(end_line)])
 


Index: clang/tools/clang-format/git-clang-format
===
--- clang/tools/clang-format/git-clang-format
+++ clang/tools/clang-format/git-clang-format
@@ -321,8 +321,9 @@
   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))
+  if line_count == 0:
+line_count = 1
+  matches.setdefault(filename, []).append(Range(start_line, line_count))
   return matches
 
 
Index: clang/tools/clang-format/clang-format-diff.py
===
--- clang/tools/clang-format/clang-format-diff.py
+++ clang/tools/clang-format/clang-format-diff.py
@@ -90,9 +90,11 @@
   line_count = 1
   if match.group(3):
 line_count = int(match.group(3))
-  if line_count == 0:
-continue
-  end_line = start_line + line_count - 1
+  # Also format lines range if line_count is 0 in case of deleting
+  # surrounding statements.
+  end_line = start_line
+  if line_count != 0:
+end_line += line_count - 1
   lines_by_file.setdefault(filename, []).extend(
   ['-lines', str(start_line) + ':' + str(end_line)])
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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`

  
  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 , 
const DataLayout ,
   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 

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 , const 
DataLayout ,
  -  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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Hans Wennborg via Phabricator via cfe-commits]

hans accepted this revision.
hans added a comment.
This revision is now accepted and ready to land.

lgtm

If you want to look into it, I think having some kind of test for this script 
would be useful.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111273

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Zequan Wu via Phabricator via cfe-commits]

zequanwu added a comment.

In D111273#3047759 , @hans wrote:

> Are there no tests for clang-format-diff.py? That seems unfortunate.

No test for clang-format-diff.py.

> Will this handle more nested cases, for example if the first line here is 
> deleted, will we fix the indent of both "if (b)" and "c"?
>
>   if (a)
> if (b)
>   c;
>
> I worry that this might be a tricky problem..

It handles this. Because `git diff -U0` gives 0 context lines, deleting lines 
will always gives 0 line_count.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111273

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

Are there no tests for clang-format-diff.py? That seems unfortunate.

Will this handle more nested cases, for example if the first line here is 
deleted, will we fix the indent of both "if (b)" and "c"?

  if (a)
if (b)
  c;

I worry that this might be a tricky problem..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111273

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Zequan Wu via Phabricator via cfe-commits]

zequanwu created this revision.
zequanwu added reviewers: rnk, phosek.
zequanwu requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

If we only delete lines that are outer block statements (if, while, etc),
clang-format-diff.py can't format the statements inside the block statements.

An example to repro:

1. Delete the if statment at line 118 in llvm/lib/CodeGen/Analysis.cpp.
2. Run `git diff -U0 --no-color HEAD^ | 
clang/tools/clang-format/clang-format-diff.py -i -p1`

It fails to format the statement after if.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D111273

Files:
  clang/tools/clang-format/clang-format-diff.py


Index: clang/tools/clang-format/clang-format-diff.py
===
--- clang/tools/clang-format/clang-format-diff.py
+++ clang/tools/clang-format/clang-format-diff.py
@@ -90,9 +90,11 @@
   line_count = 1
   if match.group(3):
 line_count = int(match.group(3))
-  if line_count == 0:
-continue
-  end_line = start_line + line_count - 1
+  # Also format lines range if line_count is 0 in case of deleting
+  # surrounding statements.
+  end_line = start_line
+  if line_count != 0:
+end_line += line_count - 1
   lines_by_file.setdefault(filename, []).extend(
   ['-lines', str(start_line) + ':' + str(end_line)])
 


Index: clang/tools/clang-format/clang-format-diff.py
===
--- clang/tools/clang-format/clang-format-diff.py
+++ clang/tools/clang-format/clang-format-diff.py
@@ -90,9 +90,11 @@
   line_count = 1
   if match.group(3):
 line_count = int(match.group(3))
-  if line_count == 0:
-continue
-  end_line = start_line + line_count - 1
+  # Also format lines range if line_count is 0 in case of deleting
+  # surrounding statements.
+  end_line = start_line
+  if line_count != 0:
+end_line += line_count - 1
   lines_by_file.setdefault(filename, []).extend(
   ['-lines', str(start_line) + ':' + str(end_line)])
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits