[PATCH] [WIP] clang-format: Enforce EOL at EOF as required by C standard

[Alp Toker]

On 06/10/2013 15:09, Daniel Jasper wrote:
> I think the problem is that to me this is not a formatting decision
> (although this might be irrational). To me, it is much closer to a
> file encoding decision. Also, I don't think this decision is part of
> any of the coding styles you have mentioned (at least not to a point
> where somebody has bothered to write it into the style guide).
I've thought about this and disagree that it's related to encoding.

Encoding is something that can be modified trivially with a tool that
doesn't know how to tokenise the programming language, which isn't the
case here.

The decision on whether to add / remove whitespace on the last line is
very much dependent on the token stream, (potentially detected) CR/CRLF
setting.

The only tool we found that could get this right prior to clang-format
was vim's syntax support in batch mode -- certainly it wasn't possible
with sed to due corner cases (we format a lot of source code!)

>
> However, I don't disagree with the fundamental concept of clang-format
> inserting these newlines. What I somewhat doubt is that it is useful
> as part of the clang-format library. If I write a tool that refactors
> a certain piece of code (e.g. as done in the unit tests), I do not
> want clang-format (more precisely libFormat as "clang-format" is the
> binary) to mess with my line-end/file-end decisions. I can very well
> use the library to format small bits and pieces of code that aren't
> even meant to be full files.

For the same reason as above, I don't think it would be possible to
implement this correctly without access to the token stream and clang
preprocessor.

You'd have to re-lex at least the last few lines of the file, and also
reimplement the touched-lines logic in libFormat unless I'm missing
something?

I also note that this patch _doesn't_ modify the newline at EOF during
refactoring, just as the tool doesn't modify other unrelated code. It
only makes a change if you've explicitly told it to modify the last line.

Finally, there are very, very few source files "in the wild" without a
terminating newline. I think the unit tests aren't realistic in that
respect which is why I suggested updating them rather than just adding a
hack and handling them with forSnippets() for the tests.

>
> Therefore, I think this particular piece of logic belongs in
> clang/tools/clang-format/ClangFormat.cpp. Right before saving a file
> (or printing to stdout), we could ensure that there is a newline at
> the end (possibly guarded on a style option or command line flag).

The approach you suggest would force the newline at EOF in all cases,
causing unwanted changes even if the user only requested to reformat a
specific line range such as in an IDE or with clang-format-diff.py.

Alp.




>
> On Sun, Oct 6, 2013 at 3:44 PM, Alp Toker <alp at nuanti.com
> <mailto:alp at nuanti.com>> wrote:
>
>     On 06/10/2013 14:13, Daniel Jasper wrote:
>     > Actually, the C++11 standard says:
>     > "A source file that is not empty and that does not end in a new-line
>     > character, or that ends in a new-line character immediately preceded
>     > by a backslash character before any such splicing takes place, shall
>     > be processed as if an additional new-line character were appended to
>     > the file."
>
>     You're right that it's not a hard error in C++, only C, and the
>     remarks
>     should be updated to reflect that.
>
>     >
>     > I think this pretty much implies that source files are allowed
>     to not
>     > end in a newline. Also, I think it is fine for clang-format to not
>     > alter its input file in this regard. If your VCS requires this,
>     > configure your editor appropriately ..
>
>     If my editor got formatting right, I wouldn't need clang-format :-)
>
>     Newline at EOF is certainly part of the coding style on the WebKit
>     project and checked for during code review. It's always a pain to miss
>     this and have to go back and fix after the fact.
>
>     I can't speak for LLVM but Benjamin Kramer's commit r191857 (Add
>     newline
>     at eof) suggests it's also part of the coding style here.
>
>     Then there's clang r189110 (Respect -Wnewline-eof even in C++11 mode,
>     <rdar://problem/12922063>) showing that Apple customers also consider
>     this important with C++ code.
>
>     I'm guessing from your response that it's not part of the Google
>     coding
>     style, so I'll make sure that this check is disabled for C++
>     sources in
>     GoogleStyle in the next revision of the patch.
>
>     That leaves ChromiumStyle which I don't know enough about. Anyone?
>
>     Thanks for the feedback.
>
>     Alp.
>
>
>     >
>     >
>     > On Sun, Oct 6, 2013 at 2:10 PM, Alp Toker <alp at nuanti.com
>     <mailto:alp at nuanti.com>
>     > <mailto:alp at nuanti.com <mailto:alp at nuanti.com>>> wrote:
>     >
>     >     Hi,
>     >
>     >     I'm just putting this patch out there for now as it's been
>     useful
>     >     to us
>     >     in its current state. Some tweaks are needed before this can
>     land:
>     >
>     >     1) It needs a lot of existing unit tests to be fixed. The tests
>     >     could be
>     >     fixed before this patch goes in if anyone has the time to
>     volunteer(!)
>     >
>     >     2) I'm not convinced it's worth even having a 'AllowNoEOL'
>     format flag
>     >     for this. The spec is clear that it's mandatory and many VCS
>     also
>     >     expect
>     >     EOL at EOF to work properly.
>     >
>     >     3) That means we might do better to add a command line flag to
>     >     identify
>     >     that the code is being formatted for snippets, which uses the
>     >     forSnippets() internally, in which case AllowNoEOL would be
>     made true.
>     >
>     >     4) There are other rules such as trailing slash in 5.5.1.2 that
>     >     I'm not
>     >     sure are handled yet, though these could be worked on
>     subequent to
>     >     this
>     >     patch landing.
>     >
>     >     Note also that this fixes a bug in clang-format where it was
>     reporting
>     >     an error code for an empty (zero-sized) file. There's no benefit
>     >     to the
>     >     special case so we can just remove that check now.
>     >
>     >     Here's the change:
>     >
>     >         clang-format: Enforce EOL at EOF as required by C standard
>     >
>     >         This enables end-of-line correction as specified for
>     C-family
>     >     languages.
>     >
>     >         C11 5.5.1.2p2 "A source file that is not empty shall end
>     in a
>     >     new-line
>     >         character"
>     >
>     >         The new setting 'AllowNoEOL' and the fluent
>     >     FormatStyle::forSnippets() helper
>     >         are provided for consumers that prefer the old
>     behaviour, which is
>     >     preferable
>     >         for inline code snippets in documentation.
>     >
>     >     Alp.
>     >
>     >     --
>     >     http://www.nuanti.com
>     >     the browser experts
>     >
>     >
>
>     --
>     http://www.nuanti.com
>     the browser experts
>
>

-- 
http://www.nuanti.com
the browser experts