Re: RFR: 8311043: Remove trailing blank lines in source files
On Wed, 28 Jun 2023 16:54:51 GMT, Leo Korinth wrote: > Remove trailing "blank" lines in source files. > > I like to use global-whitespace-cleanup-mode, but I can not use it if the > files are "dirty" to begin with. This fix will make more files "clean". I > also considered adding a check for this in jcheck for Skara, however it seems > jcheck code handling hunks does not track end-of-files. Thus I will only > clean the files. > > The fix removes trailing lines matching ^[[:space:]]*$ in > > - *.java > - *.cpp > - *.hpp > - *.c > - *.h > > I have applied the following bash script to each file: > > file="$1" > > while [[ $(tail -n 1 "$file") =~ ^[[:space:]]*$ ]]; do > truncate -s -1 "$file" > done > > `git diff --ignore-space-change --ignore-blank-lines master` displays no > changes > `git diff --ignore-blank-lines master` displays one change Ending the file with a blank line? I would not have expected that at all. I expect a single EOL at the end of the file; from a visual POV, the last line of non-blank text ends with an EOL. No more, no less. - PR Comment: https://git.openjdk.org/jdk/pull/14698#issuecomment-1614806396
Re: RFR: 8311043: Remove trailing blank lines in source files
On Thu, 29 Jun 2023 13:05:58 GMT, Leo Korinth wrote: > My changes look like this in the diff output > ``` > } > - > ``` Thanks for showing this and other output. To me this looked like the final newline had been removed. I would have expected to see something that more obviously showed more than one blank line before hand and exactly one after. - PR Comment: https://git.openjdk.org/jdk/pull/14698#issuecomment-1613985636
Re: RFR: 8311043: Remove trailing blank lines in source files
On Wed, 28 Jun 2023 16:54:51 GMT, Leo Korinth wrote: > Remove trailing "blank" lines in source files. > > I like to use global-whitespace-cleanup-mode, but I can not use it if the > files are "dirty" to begin with. This fix will make more files "clean". I > also considered adding a check for this in jcheck for Skara, however it seems > jcheck code handling hunks does not track end-of-files. Thus I will only > clean the files. > > The fix removes trailing lines matching ^[[:space:]]*$ in > > - *.java > - *.cpp > - *.hpp > - *.c > - *.h > > I have applied the following bash script to each file: > > file="$1" > > while [[ $(tail -n 1 "$file") =~ ^[[:space:]]*$ ]]; do > truncate -s -1 "$file" > done > > `git diff --ignore-space-change --ignore-blank-lines master` displays no > changes > `git diff --ignore-blank-lines master` displays one change This was not liked, I will close it. I will possibly do it under another PR for the GC code. Thanks for reviewing. - PR Comment: https://git.openjdk.org/jdk/pull/14698#issuecomment-1613526703
Re: RFR: 8311043: Remove trailing blank lines in source files
On Wed, 28 Jun 2023 16:54:51 GMT, Leo Korinth wrote: > Remove trailing "blank" lines in source files. > > I like to use global-whitespace-cleanup-mode, but I can not use it if the > files are "dirty" to begin with. This fix will make more files "clean". I > also considered adding a check for this in jcheck for Skara, however it seems > jcheck code handling hunks does not track end-of-files. Thus I will only > clean the files. > > The fix removes trailing lines matching ^[[:space:]]*$ in > > - *.java > - *.cpp > - *.hpp > - *.c > - *.h > > I have applied the following bash script to each file: > > file="$1" > > while [[ $(tail -n 1 "$file") =~ ^[[:space:]]*$ ]]; do > truncate -s -1 "$file" > done > > `git diff --ignore-space-change --ignore-blank-lines master` displays no > changes > `git diff --ignore-blank-lines master` displays one change Per had an emacs feature to remove whitespaces at the end of the line, and gave me the vim version of that. That's a nice feature. I object to this change. - PR Comment: https://git.openjdk.org/jdk/pull/14698#issuecomment-1613437709
Re: RFR: 8311043: Remove trailing blank lines in source files
On Thu, 29 Jun 2023 12:40:34 GMT, Coleen Phillimore wrote: > You could fix your emacs functions. It is a *very nice* feature of global-whitespace-cleanup-mode - PR Comment: https://git.openjdk.org/jdk/pull/14698#issuecomment-1613252347
Re: RFR: 8311043: Remove trailing blank lines in source files
On Thu, 29 Jun 2023 12:11:40 GMT, David Holmes wrote: > Neither the PR diffs nor the webrev make it easy to see exactly what is being > changed here. It appeared to me that the last empty line of each file was > being deleted, leaving no newline at the end. My changes look like this in the diff output } - Removal of the last newline would look like this: -} +} \ No newline at end of file (both with `git diff` and `git diff --unified`) I have not tested if this is also true for the generated webrevs, but I think that is precisely how they are created. - PR Comment: https://git.openjdk.org/jdk/pull/14698#issuecomment-1613152641
Re: RFR: 8311043: Remove trailing blank lines in source files
On Wed, 28 Jun 2023 16:54:51 GMT, Leo Korinth wrote: > Remove trailing "blank" lines in source files. > > I like to use global-whitespace-cleanup-mode, but I can not use it if the > files are "dirty" to begin with. This fix will make more files "clean". I > also considered adding a check for this in jcheck for Skara, however it seems > jcheck code handling hunks does not track end-of-files. Thus I will only > clean the files. > > The fix removes trailing lines matching ^[[:space:]]*$ in > > - *.java > - *.cpp > - *.hpp > - *.c > - *.h > > I have applied the following bash script to each file: > > file="$1" > > while [[ $(tail -n 1 "$file") =~ ^[[:space:]]*$ ]]; do > truncate -s -1 "$file" > done > > `git diff --ignore-space-change --ignore-blank-lines master` displays no > changes > `git diff --ignore-blank-lines master` displays one change You could fix your emacs functions. - PR Comment: https://git.openjdk.org/jdk/pull/14698#issuecomment-1613106245
Re: RFR: 8311043: Remove trailing blank lines in source files
On Thu, 29 Jun 2023 12:01:03 GMT, Coleen Phillimore wrote: > Why do we care about this? I care because of global-whitespace-cleanup-mode (in emacs). It helps me remove trailing whitespaces and blanklines when saving but it will not fix a file that was "dirty" when it was opened. Trailing blank lines triggers it not to clean whitespaces for me. And it does not look good. - PR Comment: https://git.openjdk.org/jdk/pull/14698#issuecomment-1613095390
Re: RFR: 8311043: Remove trailing blank lines in source files
On Wed, 28 Jun 2023 16:54:51 GMT, Leo Korinth wrote: > Remove trailing "blank" lines in source files. > > I like to use global-whitespace-cleanup-mode, but I can not use it if the > files are "dirty" to begin with. This fix will make more files "clean". I > also considered adding a check for this in jcheck for Skara, however it seems > jcheck code handling hunks does not track end-of-files. Thus I will only > clean the files. > > The fix removes trailing lines matching ^[[:space:]]*$ in > > - *.java > - *.cpp > - *.hpp > - *.c > - *.h > > I have applied the following bash script to each file: > > file="$1" > > while [[ $(tail -n 1 "$file") =~ ^[[:space:]]*$ ]]; do > truncate -s -1 "$file" > done > > `git diff --ignore-space-change --ignore-blank-lines master` displays no > changes > `git diff --ignore-blank-lines master` displays one change Neither the PR diffs nor the webrev make it easy to see exactly what is being changed here. It appeared to me that the last empty line of each file was being deleted, leaving no newline at the end. But to me this is too disruptive with no tangible benefit. And you need buy-in from all the different areas affected by this. - PR Comment: https://git.openjdk.org/jdk/pull/14698#issuecomment-1613043398
Re: RFR: 8311043: Remove trailing blank lines in source files
On Wed, 28 Jun 2023 16:54:51 GMT, Leo Korinth wrote: > Remove trailing "blank" lines in source files. > > I like to use global-whitespace-cleanup-mode, but I can not use it if the > files are "dirty" to begin with. This fix will make more files "clean". I > also considered adding a check for this in jcheck for Skara, however it seems > jcheck code handling hunks does not track end-of-files. Thus I will only > clean the files. > > The fix removes trailing lines matching ^[[:space:]]*$ in > > - *.java > - *.cpp > - *.hpp > - *.c > - *.h > > I have applied the following bash script to each file: > > file="$1" > > while [[ $(tail -n 1 "$file") =~ ^[[:space:]]*$ ]]; do > truncate -s -1 "$file" > done > > `git diff --ignore-space-change --ignore-blank-lines master` displays no > changes > `git diff --ignore-blank-lines master` displays one change Why do we care about this? - PR Comment: https://git.openjdk.org/jdk/pull/14698#issuecomment-1613018234
Re: RFR: 8311043: Remove trailing blank lines in source files
On Thu, 29 Jun 2023 07:41:11 GMT, David Holmes wrote: > This seems to run contrary to the requirement that files end in a newline, > which git will complain about if the newline is missing. > > It also seems far too large and disruptive. Do you still think it is too disruptive after Erik's explanation? I could split it in more reviews, but I thought that maybe it would just make the review harder. I was hoping the two `git diff` commands I gave in my first comment in combination with the clean script would make the change seem reviewable. - PR Comment: https://git.openjdk.org/jdk/pull/14698#issuecomment-1612660457
Re: RFR: 8311043: Remove trailing blank lines in source files
On Thu, 29 Jun 2023 07:41:11 GMT, David Holmes wrote: > This seems to run contrary to the requirement that files end in a newline, > which git will complain about if the newline is missing. The patch is leaving exactly one newline at the end of the file. - PR Comment: https://git.openjdk.org/jdk/pull/14698#issuecomment-1612588091
Re: RFR: 8311043: Remove trailing blank lines in source files
On Wed, 28 Jun 2023 16:54:51 GMT, Leo Korinth wrote: > Remove trailing "blank" lines in source files. > > I like to use global-whitespace-cleanup-mode, but I can not use it if the > files are "dirty" to begin with. This fix will make more files "clean". I > also considered adding a check for this in jcheck for Skara, however it seems > jcheck code handling hunks does not track end-of-files. Thus I will only > clean the files. > > The fix removes trailing lines matching ^[[:space:]]*$ in > > - *.java > - *.cpp > - *.hpp > - *.c > - *.h > > I have applied the following bash script to each file: > > file="$1" > > while [[ $(tail -n 1 "$file") =~ ^[[:space:]]*$ ]]; do > truncate -s -1 "$file" > done > > `git diff --ignore-space-change --ignore-blank-lines master` displays no > changes > `git diff --ignore-blank-lines master` displays one change This seems to run contrary to the requirement that files end in a newline, which git will complain about if the newline is missing. It also seems far too large and disruptive. - PR Comment: https://git.openjdk.org/jdk/pull/14698#issuecomment-1612567676
