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

[Alp Toker]

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

-------------- next part --------------
commit c2bfc4d5399d83ec19a3db1e321e6faa04e798b3
Author: Alp Toker <alp at nuanti.com>
Date:   Sun Oct 6 16:54:05 2013 +0100

    clang-format: Add unit tests for EOL-at-EOF

diff --git a/unittests/Format/FormatTest.cpp b/unittests/Format/FormatTest.cpp
index 85813bc..d1d2e79 100644
--- a/unittests/Format/FormatTest.cpp
+++ b/unittests/Format/FormatTest.cpp
@@ -20,20 +20,25 @@ namespace format {
 class FormatTest : public ::testing::Test {
 protected:
   std::string format(llvm::StringRef Code, unsigned Offset, unsigned Length,
-                     const FormatStyle &Style) {
+                     FormatStyle Style, bool asSnippet = true) {
+    // Most tests are standalone code snippets so don't format them as complete
+    // source files.
+    if (asSnippet)
+      Style = Style.forSnippets();
+
     DEBUG(llvm::errs() << "---\n");
     std::vector<tooling::Range> Ranges(1, tooling::Range(Offset, Length));
     tooling::Replacements Replaces = reformat(Style, Code, Ranges);
     ReplacementCount = Replaces.size();
     std::string Result = applyAllReplacements(Code, Replaces);
-    EXPECT_NE("", Result);
     DEBUG(llvm::errs() << "\n" << Result << "\n\n");
     return Result;
   }
 
-  std::string
-  format(llvm::StringRef Code, const FormatStyle &Style = getLLVMStyle()) {
-    return format(Code, 0, Code.size(), Style);
+  std::string format(llvm::StringRef Code,
+                     const FormatStyle &Style = getLLVMStyle(),
+                     bool asSnippet = true) {
+    return format(Code, 0, Code.size(), Style, asSnippet);
   }
 
   std::string messUp(llvm::StringRef Code) {
@@ -6379,6 +6384,7 @@ TEST_F(FormatTest, ParsesConfiguration) {
   CHECK_PARSE_BOOL(SpacesInCStyleCastParentheses);
   CHECK_PARSE_BOOL(SpaceAfterControlStatementKeyword);
   CHECK_PARSE_BOOL(SpaceBeforeAssignmentOperators);
+  CHECK_PARSE_BOOL(AllowNoEOL);
 
   CHECK_PARSE("AccessModifierOffset: -1234", AccessModifierOffset, -1234);
   CHECK_PARSE("ConstructorInitializerIndentWidth: 1234",
@@ -6751,5 +6757,19 @@ TEST_F(FormatTest, SupportsCRLF) {
                    getGoogleStyle()));
 }
 
+TEST_F(FormatTest, FixesEOLAtEOF) {
+  EXPECT_EQ("", format("", getLLVMStyle().forSnippets(), false));
+  EXPECT_EQ("", format("", getLLVMStyle(), false));
+
+  EXPECT_EQ("a", format("a", getLLVMStyle().forSnippets(), false));
+  EXPECT_EQ("a\n", format("a", getLLVMStyle(), false));
+
+  EXPECT_EQ("a\n", format("a\n", getLLVMStyle().forSnippets(), false));
+  EXPECT_EQ("a\n", format("a\n", getLLVMStyle(), false));
+
+  EXPECT_EQ("a\n", format("a\n\n", getLLVMStyle().forSnippets(), false));
+  EXPECT_EQ("a\n", format("a\n\n", getLLVMStyle(), false));
+}
+
 } // end namespace tooling
 } // end namespace clang