[PATCH] D76428: [cmake] Disable C4129 warning for MSVC.

[Adrian McCarthy via Phabricator via llvm-commits]

amccarth added a comment.

I'm reluctant to globally disable a useful warning given that the false positives problem has already been fixed for a while (since, at least, MSVC 2017).

In older versions of MSVC, I believe this will actually miscompile, so the "false positive" warning is actually useful.

The issue, as I understand it, comes in code like this:

    EXPECT_EQ(R"(digraph VPlan {
  graph [labelloc=t, fontsize=30; label="Vectorization Plan"]
  node [shape=rect, fontname=Courier, fontsize=30]
  edge [fontname=Courier, fontsize=30]
  compound=true
    N0 [label =
      ":\n" +
        "EMIT %vp0 = catchswitch\l" +
        "EMIT %vp1 = ret %vp0\l" +
        "EMIT %vp2 = br %vp0 %vp1\l"
    ]
    N0 -> N1 [ label=""]
    N1 [label =
      ":\n" +
        "EMIT %vp3 = indirectbr %vp2 %vp1\l" +
        "EMIT %vp4 = invoke %vp0\l"
    ]
  }
  )",
              FullDump);

The older MSVC preprocessor didn't tokenize raw string literals, so it treated the raw string as a big blob.  This bug was fixed in MSVC 2017.

EXPECT_EQ is an assert-like macro that uses the preprocessor's `#` operator to stringify its arguments.  Since the raw string hadn't yet been tokenized, the stringify effectively just put quotation marks around the literal representation of the raw string, creating a regular string literal that contains invalid junk like the unrecognized escape sequences and possibly problems with the internal quotation marks.  In effect, this creates a miscompile because the message EXPECT_EQ would display upon failure would be garbled.

At a minimum, I'd like to see the comment changed to say "... because MSVC before 2017 warns ...".

A TODO would be nice so that we know to can re-enable the warning after LLVM progresses to MSVC 2017 as a baseline.

Another option would be to change the code to avoid using raw strings as arguments to macros that are going to be stringified.  This means EXPECT_EQ failure messages _might_ be a tiny bit harder to understand.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76428/new/

https://reviews.llvm.org/D76428