[WIP] Reclassify form-feed and vertical tab as vertical WS for the purposes of lexing.

[cor3ntin]

Summary

According to The Unicode standard, line feed and vertical tab
should be considered line breaks.

See notably http://unicode.org/reports/tr14/#BK

In addition, the C++ standard stipulates:

If there is a form-feed or a vertical-tab character in such a comment,
only whitespace characters shall appear between it
and the new-line that terminates the comment; no diagnostic is required.

Given the amount of ?? I found in the test, it isn't clear that
it was intended for these characters not to be considered
vertical spaces.

This came up in SG16/WG21 in the context of
https://wg21.link/p2348.

[cor3ntin]

Fix tests

[dexonsmith]

Thanks for looking at this — I'm just reviewing the test changes in the minimizer (assuming the code change is the right thing to do).

clang/unittests/Lex/DependencyDirectivesSourceMinimizerTest.cpp

170–175

Please don't modify this assertion to test \v and \f. Instead add new assertions. I'm not sure if they belong in DefineMultilineArgs, or if DefineSpacing might be a better spot, or maybe a new test DefineVerticalWhitespace?

186

Please don't remove this test coverage.

[cor3ntin]

clang/unittests/Lex/DependencyDirectivesSourceMinimizerTest.cpp

170–175

I think this test is useful, you are right I should add more somewhere else. I will also add that new test without removing the old one!

[cor3ntin]

• Add tests and fix existing tests (again)

[xgupta]

(If you pass --draft flag to arc while updating revision it becomes WIP so you don't need to retitle or remove reviewers. Buildbot also work in that case. And to again ask for review you can use Add Action drop down menu.)

[cor3ntin]

Thanks for the tip!

[hubert.reinterpretcast]

I find the lack of changes to tests other than API unittests to be somewhat concerning.
Indeed, I applied the patch and did not notice behaviour changes in how \f or \v were handled in various contexts sensitive to line-termination.

For example:

clang -fsyntax-only -xc -<<<$'#define X\f int\n#define Y\v x;\ncha\\\v\nr x;\n// \fextern int x;\n'
<stdin>:3:5: warning: backslash and newline separated by space [-Wbackslash-newline-escape]
cha\<U+000B>
    ^
1 warning generated.

It appears phase 3 whitespace conversion is applied to \f and \v and the macro definitions extend to the \ns.
The \v\n sequence is treated as only one newline (backslash escaped) and not two (with only the \v escaped).
It also appears that the // comment is not terminated by the \f.

[rsmith]

I assume this is intended to form part of the implementation of https://wg21.link/p2348 and so shouldn't be considered for review right now?

Drive-by observation: under P2348, Clang's behavior of treating \n\r as a single new-line would be "non-standard" (requiring special phase 1 mapping). Is that intentional? \n\r is used as a new-line character on old Mac systems.

[cor3ntin]

In D108742#2970218, @hubert.reinterpretcast wrote:

I find the lack of changes to tests other than API unittests to be somewhat concerning.
Indeed, I applied the patch and did not notice behaviour changes in how \f or \v were handled in various contexts sensitive to line-termination.

For example:

clang -fsyntax-only -xc -<<<$'#define X\f int\n#define Y\v x;\ncha\\\v\nr x;\n// \fextern int x;\n'
<stdin>:3:5: warning: backslash and newline separated by space [-Wbackslash-newline-escape]
cha\<U+000B>
    ^
1 warning generated.

It appears phase 3 whitespace conversion is applied to \f and \v and the macro definitions extend to the \ns.
The \v\n sequence is treated as only one newline (backslash escaped) and not two (with only the \v escaped).
It also appears that the // comment is not terminated by the \f.

Yep, I noticed that today, \n is hardcoded in many places in the lexer, as it turns out, Hence why I marked the change WIP until I reassess the situation.
I should have been more suspicious than no test broke...

I also observed that gcc behavior seems somewhat consistent with clang.

I'm starting to fear that we might not be able to align compilers to consistency with Unicode.
Also as far as comments are concerned clang is strictly non-conforming (but neither seems to be gcc ?) - or rather as per the standard it produces ill-formed NDR programs, which is interesting.

It might be that the best solution is to abandon this route and instead treat \v and \f as whitespace rather than new-line in the standard and be inconsistent with Unicode.
The is probably not a huge motivation for having \v and \f being line breaking anyway (beyond Unicode saying so).

[cor3ntin]

In D108742#2970263, @rsmith wrote:

I assume this is intended to form part of the implementation of https://wg21.link/p2348 and so shouldn't be considered for review right now?

Yes, I wanted your feedback anyway but that was before I realized \n was hardcoded in many places in the lexer, notably in comment parsing. Which makes me question my entire reasoning. Sorry for the noise.

Drive-by observation: under P2348, Clang's behavior of treating \n\r as a single new-line would be "non-standard" (requiring special phase 1 mapping). Is that intentional? \n\r is used as a new-line character on old Mac systems.

Somewhat. \n\r is not described by Unicode so we could either mandate that all implementation support that or leave it as implementation-defined mapping. Correct me if I am wrong, but as the line number is itself implementation-defined, whether there are one or 2 line breaks would not materially affect the standard, either way.
But also, when I looked at it, I was under the impression that older macs used a single \r.

[rsmith]

In D108742#2970283, @cor3ntin wrote:

Drive-by observation: under P2348, Clang's behavior of treating \n\r as a single new-line would be "non-standard" (requiring special phase 1 mapping). Is that intentional? \n\r is used as a new-line character on old Mac systems.

Somewhat. \n\r is not described by Unicode so we could either mandate that all implementation support that or leave it as implementation-defined mapping. Correct me if I am wrong, but as the line number is itself implementation-defined, whether there are one or 2 line breaks would not materially affect the standard, either way.

Yes, I suppose that's true. Though if we're nailing down exactly how new-lines are defined and asking every conforming implementation to support UTF-8 and such, maybe it's time to also define how the presumed line number is determined? =)

But also, when I looked at it, I was under the impression that older macs used a single \r.

Oops, you're right, I was thinking of Acorn systems not Macs :)

[cor3ntin]

In D108742#2970302, @rsmith wrote:

In D108742#2970283, @cor3ntin wrote:

Drive-by observation: under P2348, Clang's behavior of treating \n\r as a single new-line would be "non-standard" (requiring special phase 1 mapping). Is that intentional? \n\r is used as a new-line character on old Mac systems.

Somewhat. \n\r is not described by Unicode so we could either mandate that all implementation support that or leave it as implementation-defined mapping. Correct me if I am wrong, but as the line number is itself implementation-defined, whether there are one or 2 line breaks would not materially affect the standard, either way.

Yes, I suppose that's true. Though if we're nailing down exactly how new lines are defined and asking every conforming implementation to support UTF-8 and such, maybe it's time to also define how the presumed line number is determined? =)

I'm not sure that we want people to rely on the value on LINE, to be honest.

Anyway, I've been thinking a lot about that and I don't think this change is worth pursuing. I've realized all compilers treat VT and FF as non-line breaking horizontal whitespace and even if that's not consistent with Unicode it's also consistent with other programming languages.
I'll modify the paper to keep treating these codepoints as horizontal whitespaces as this seems the sane, non-disruptive course of action.

I will also try to address your feedback regarding acorn systems on the paper.

Thanks!

clang/lib/Basic/CharInfo.cpp

 //===--- CharInfo.cpp - Static Data for Classifying ASCII Characters ------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Basic/CharInfo.h"
 
 using namespace clang::charinfo;
 
 // Statically initialize CharInfo table based on ASCII character set
 // Reference: FreeBSD 7.2 /usr/share/misc/ascii
+// clang-format off
 const uint16_t clang::charinfo::InfoTable[256] = {
   // 0 NUL         1 SOH         2 STX         3 ETX
   // 4 EOT         5 ENQ         6 ACK         7 BEL
   0           , 0           , 0           , 0           ,
   0           , 0           , 0           , 0           ,
   // 8 BS          9 HT         10 NL         11 VT
   //12 NP         13 CR         14 SO         15 SI
-  0           , CHAR_HORZ_WS, CHAR_VERT_WS, CHAR_HORZ_WS,
-  CHAR_HORZ_WS, CHAR_VERT_WS, 0           , 0           ,
+  0           , CHAR_HORZ_WS, CHAR_VERT_WS, CHAR_VERT_WS,
+  CHAR_VERT_WS, CHAR_VERT_WS, 0           , 0           ,
   //16 DLE        17 DC1        18 DC2        19 DC3
   //20 DC4        21 NAK        22 SYN        23 ETB
   0           , 0           , 0           , 0           ,
   0           , 0           , 0           , 0           ,
   //24 CAN        25 EM         26 SUB        27 ESC
   //28 FS         29 GS         30 RS         31 US
   0           , 0           , 0           , 0           ,
   0           , 0           , 0           , 0           ,

clang/unittests/Basic/CharInfoTest.cpp

 using namespace llvm;
 using namespace clang;
 
 // Check that the CharInfo table has been constructed reasonably.
 TEST(CharInfoTest, validateInfoTable) {
   using namespace charinfo;
   EXPECT_EQ((unsigned)CHAR_SPACE,   InfoTable[(unsigned)' ']);
   EXPECT_EQ((unsigned)CHAR_HORZ_WS, InfoTable[(unsigned)'\t']);
-  EXPECT_EQ((unsigned)CHAR_HORZ_WS, InfoTable[(unsigned)'\f']); // ??
-  EXPECT_EQ((unsigned)CHAR_HORZ_WS, InfoTable[(unsigned)'\v']); // ??
+  EXPECT_EQ((unsigned)CHAR_VERT_WS, InfoTable[(unsigned)'\f']);
+  EXPECT_EQ((unsigned)CHAR_VERT_WS, InfoTable[(unsigned)'\v']);
   EXPECT_EQ((unsigned)CHAR_VERT_WS, InfoTable[(unsigned)'\n']);
   EXPECT_EQ((unsigned)CHAR_VERT_WS, InfoTable[(unsigned)'\r']);
   EXPECT_EQ((unsigned)CHAR_UNDER,   InfoTable[(unsigned)'_']);
   EXPECT_EQ((unsigned)CHAR_PERIOD,  InfoTable[(unsigned)'.']);
 
   for (unsigned i = 'a'; i <= 'f'; ++i) {
     EXPECT_EQ((unsigned)CHAR_XLOWER, InfoTable[i]);
     EXPECT_EQ((unsigned)CHAR_XUPPER, InfoTable[i+'A'-'a']);
TEST(CharInfoTest, isHorizontalWhitespace) {
   EXPECT_FALSE(isHorizontalWhitespace('0'));
   EXPECT_FALSE(isHorizontalWhitespace('.'));
   EXPECT_FALSE(isHorizontalWhitespace('`'));
   EXPECT_FALSE(isHorizontalWhitespace('\0'));
   EXPECT_FALSE(isHorizontalWhitespace('\x7f'));
 
   EXPECT_TRUE(isHorizontalWhitespace(' '));
   EXPECT_TRUE(isHorizontalWhitespace('\t'));
-  EXPECT_TRUE(isHorizontalWhitespace('\f')); // ??
-  EXPECT_TRUE(isHorizontalWhitespace('\v')); // ??
 
   EXPECT_FALSE(isHorizontalWhitespace('\n'));
   EXPECT_FALSE(isHorizontalWhitespace('\r'));
 
   EXPECT_FALSE(isHorizontalWhitespace('\x80'));
   EXPECT_FALSE(isHorizontalWhitespace('\xc2'));
   EXPECT_FALSE(isHorizontalWhitespace('\xff'));
 }
 
 TEST(CharInfoTest, isVerticalWhitespace) {
   EXPECT_FALSE(isVerticalWhitespace('a'));
   EXPECT_FALSE(isVerticalWhitespace('_'));
   EXPECT_FALSE(isVerticalWhitespace('0'));
   EXPECT_FALSE(isVerticalWhitespace('.'));
   EXPECT_FALSE(isVerticalWhitespace('`'));
   EXPECT_FALSE(isVerticalWhitespace('\0'));
   EXPECT_FALSE(isVerticalWhitespace('\x7f'));
 
   EXPECT_FALSE(isVerticalWhitespace(' '));
   EXPECT_FALSE(isVerticalWhitespace('\t'));
-  EXPECT_FALSE(isVerticalWhitespace('\f')); // ??
-  EXPECT_FALSE(isVerticalWhitespace('\v')); // ??
 
+  EXPECT_TRUE(isVerticalWhitespace('\f')); // ??
+  EXPECT_TRUE(isVerticalWhitespace('\v')); // ??
   EXPECT_TRUE(isVerticalWhitespace('\n'));
   EXPECT_TRUE(isVerticalWhitespace('\r'));
 
   EXPECT_FALSE(isVerticalWhitespace('\x80'));
   EXPECT_FALSE(isVerticalWhitespace('\xc2'));
   EXPECT_FALSE(isVerticalWhitespace('\xff'));
 }
 

clang/unittests/Lex/DependencyDirectivesSourceMinimizerTest.cpp

 }
 
 TEST(MinimizeSourceToDependencyDirectivesTest, DefineHorizontalWhitespace) {
   SmallVector<char, 128> Out;
 
   ASSERT_FALSE(minimizeSourceToDependencyDirectives(
       "#define MACRO(\t)\tcon \t tent\t", Out));
   EXPECT_STREQ("#define MACRO() con \t tent\n", Out.data());
-
-  ASSERT_FALSE(minimizeSourceToDependencyDirectives(
-      "#define MACRO(\f)\fcon \f tent\f", Out));
-  EXPECT_STREQ("#define MACRO() con \f tent\n", Out.data());
-
-  ASSERT_FALSE(minimizeSourceToDependencyDirectives(
-      "#define MACRO(\v)\vcon \v tent\v", Out));
-  EXPECT_STREQ("#define MACRO() con \v tent\n", Out.data());
-
-  ASSERT_FALSE(minimizeSourceToDependencyDirectives(
-      "#define MACRO \t\v\f\v\t con\f\t\vtent\v\f \v", Out));
-  EXPECT_STREQ("#define MACRO con\f\t\vtent\n", Out.data());
 }
 
 TEST(MinimizeSourceToDependencyDirectivesTest, DefineMultilineArgs) {
   SmallVector<char, 128> Out;
 
   ASSERT_FALSE(
       minimizeSourceToDependencyDirectives("#define MACRO(a        \\\n"
                                            "              )",
                                            Out));
   EXPECT_STREQ("#define MACRO(a)\n", Out.data());
 
   ASSERT_FALSE(
       minimizeSourceToDependencyDirectives("#define MACRO(a,       \\\n"
                                            "              b)       \\\n"
                                            "        call((a),      \\\n"
                                            "             (b))",
                                            Out));
   EXPECT_STREQ("#define MACRO(a,b) call((a), (b))\n", Out.data());
 }
 
 TEST(MinimizeSourceToDependencyDirectivesTest,
+     DefineMultilineArgsVerticalWhiteSpaces) {
+  SmallVector<char, 128> Out;
+
+  ASSERT_FALSE(
+      minimizeSourceToDependencyDirectives("#define MACRO(a        \\\f"
+                                           "              )",
+                                           Out));
+  EXPECT_STREQ("#define MACRO(a)\n", Out.data());
+
+  ASSERT_FALSE(
+      minimizeSourceToDependencyDirectives("#define MACRO(a        \\\v"
+                                           "              )",
+                                           Out));
+  EXPECT_STREQ("#define MACRO(a)\n", Out.data());
+
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives(
+      "#define MACRO(a        \\\f\\\v\\\v\\\f"
+      "              )",
+      Out));
+  EXPECT_STREQ("#define MACRO(a)\n", Out.data());
+}
+
+TEST(MinimizeSourceToDependencyDirectivesTest,
      DefineMultilineArgsCarriageReturn) {
   SmallVector<char, 128> Out;
 
   ASSERT_FALSE(
       minimizeSourceToDependencyDirectives("#define MACRO(a,       \\\r"
                                            "              b)       \\\r"
                                            "        call((a),      \\\r"
                                            "             (b))",

clang/unittests/Lex/LexerTest.cpp

 
 TEST_F(LexerTest, IsNewLineEscapedValid) {
   auto hasNewLineEscaped = [](const char *S) {
     return Lexer::isNewLineEscaped(S, S + strlen(S) - 1);
   };
 
   EXPECT_TRUE(hasNewLineEscaped("\\\r"));
   EXPECT_TRUE(hasNewLineEscaped("\\\n"));
+  EXPECT_TRUE(hasNewLineEscaped("\\\v"));
   EXPECT_TRUE(hasNewLineEscaped("\\\r\n"));
   EXPECT_TRUE(hasNewLineEscaped("\\\n\r"));
-  EXPECT_TRUE(hasNewLineEscaped("\\ \t\v\f\r"));
-  EXPECT_TRUE(hasNewLineEscaped("\\ \t\v\f\r\n"));
+  EXPECT_TRUE(hasNewLineEscaped("\\ \t\r\n"));
 
   EXPECT_FALSE(hasNewLineEscaped("\\\r\r"));
   EXPECT_FALSE(hasNewLineEscaped("\\\r\r\n"));
   EXPECT_FALSE(hasNewLineEscaped("\\\n\n"));
   EXPECT_FALSE(hasNewLineEscaped("\r"));
   EXPECT_FALSE(hasNewLineEscaped("\n"));
   EXPECT_FALSE(hasNewLineEscaped("\r\n"));
   EXPECT_FALSE(hasNewLineEscaped("\n\r"));