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

[Alp Toker]

On 06/10/2013 19:33, Daniel Jasper wrote:
> I think I still can't follow most of your reasoning.
>
> There are several different points:
> - Detection of <CR><LF> vs. <LF>: Yes, that is something I hadn't
> though of, but there are many ways to solve this.

Such as? A good, well-tested solution exists in libFormat :-)

> - Adding the newline needs token stream information: I don't
> understand why.

Detection of the trailing slash, at least will benefit from the token
stream. I guess a solution can be engineered for that too but it'd be
more code and more tests to write.

> - Adding the newline should only be done when the last line is
> formatted: I disagree. I can see some reason for always doing this
> (guarded e.g. on a command line flag). Or alternatively, I think only
> adding the trailing newline if the entire file is formatted (i.e. not
> formatting ranges are used) should solve most use cases.

This is actually the main use-case I'm interested in. If a refactoring
tool or, say, the clang FixIt applicator removes or adds to the end of
the file which previously terminated with EOF, most open source projects
would expect you to fix the whitespace violation in the patch.

As a thought experiment, consider the base-case: A completely empty
file, into which a clang-based refactoring tool is inserting a single
statement. In this case, you'd end up with a no-EOL-terminated file in
the current state of things -- unlikely to be what the user wanted. The
base-case never lies ;-)

For all other cases, we don't want to touch the last line as it's "not
our problem".

As for reformatting the entire source file, it might be nice to fix the
violation there as well (and this happens automatically with my patch)
but I don't have a strong opinion on this if you want to disable it.

> In practice, people will rarely format just the last line (as it
> doesn't normally contain anything format-worthy).
> - libFormat will have to make the distinction between snippets and
> full files sooner or later: I am not saying that I am sure that you
> are wrong, but I also haven't seen a good reason for this yet.

The tools I'm most interested already use, or will likely use libFormat
directly, typically with a Rewriter and SourceManager instance. Also
interested in clang-format-diff.py but have never needed to run on an
entire source file start to finish yet.

>
> The way I see it currently is:
> - libFormat handles snippets of code. There is no relation to files.
> It doesn't read files, it doesn't write files, ... Thus, whether a
> code snippet ends with a newline or not is of no concern to libFormat.
> - clang-format (ClangFormat.cpp) uses libFormat to format files. It
> should be able to fix missing EOF-EOLs.

Interested to see how this would be implementated.

>
> I am sorry that you regard the response as negative.. I probably just
> don't understand some of your reasoning and am thus leaning towards
> solving this in a slightly different fashion.

I see where you're coming from but it doesn't quite work for solving the
tooling problems AFAICT.

So, I think this topic is worth re-visiting some weeks down the line
once we've had a chance to digest the matter.

Alp.



>
> Cheers,
> Daniel
>
>
> On Sun, Oct 6, 2013 at 6:34 PM, Alp Toker <alp at nuanti.com
> <mailto:alp at nuanti.com>> wrote:
>
>
>     On 06/10/2013 15:09, Daniel Jasper wrote:
>     >
>     > 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.
>
>     On this point, I think libFormat will have to make the distinction
>     sooner or later between formatting standalone code snippets and entire
>     source files. There are various constructs that you want to handle
>     differently at the beginning or end of a file, with EOL-at-EOF
>     being one
>     that gets lots of air time in the C/C++ language specifications.
>
>     This is why the patch provides forSnippets(), and libclang's CXComment
>     has been updated to use it:
>
>     /// \brief Adjust the configuration to format single-line code chunks.
>     ///
>     /// The updated format will not enforce trailing EOL at EOF and
>     may also
>     /// apply other adjustments tailored for short inline code snippets
>     appearing
>     /// in technical documentation.
>     FormatStyle forSnippets() const;
>
>     I'm attaching the corresponding unit tests -- all the existing tests
>     pass fine without modification. It's my fault for not having
>     communicated this better.
>
>     The trouble with having the feature switched off by default, then
>     overriding it for complete source files is that it would then be
>     impossible for the user to switch it off in a configuration file.
>
>     Counterintuitive as it is, I think the most gentle way to add this
>     feature is to:
>
>      * Have it on by default.
>      * Disable it explicitly for code snippets such as unit tests and
>     documentation using forSnippets().
>      * Permit the user to switch it off in their configuration file if
>     they
>     really want for some reason.
>
>     This is the only solution I could see that sets us in the right
>     direction to handle the trailing slash requirements in the C spec
>     (which
>     are also soft warnings in C++) but maybe there's another way?
>
>     Given the negative response I'll keep this changeset on our internal
>     branch and maybe we can revisit it for inclusion in Open Source
>     clang in
>     future once there's more data available.
>
>     Regards,
>     Alp.
>
>     --
>     http://www.nuanti.com
>     the browser experts
>
>

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