[PATCH] D135972: [clang-format] Don't crash on malformed preprocessor conditions

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added inline comments.


================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:1217
+  assert(PPBranchLevel >= -1);
+  if (PPBranchLevel <= -1)
+    conditionalCompilationStart(/*Unreachable=*/true);
----------------
You assert >= -1, that means this has to be == -1.


================
Comment at: clang/unittests/Format/FormatTest.cpp:5196-5206
+  std::function<void(std::string, unsigned)> FormatBadBranches =
+      [&](std::string Prefix, unsigned Lines) {
+        const std::string Directives[] = {"", "#if X\n", "#else X\n",
+                                          "#endif\n"};
+        if (Lines == 0)
+          verifyNoCrash(Prefix);
+        else
----------------
sstwcw wrote:
> owenpan wrote:
> > Can we have individual `verifyFormat` or `verifyNoCrash`tests for easy reading and to avoid the overhead? Some examples:
> > ```
> > #else
> > a;
> > #if X
> > b;
> > #endif
> > #endif
> > 
> > #elif X
> > a;
> > #endif
> > #ifdef X
> > b;
> > #endif
> > ```
> I added some tests like the examples.  But I kept the generated cases.  I feel more secure that way.  At first I only found the bug because of the generated cases.  As for overhead, the debug build of this test took 1.6s on my laptop.
> I added some tests like the examples.  But I kept the generated cases.  I feel more secure that way.  At first I only found the bug because of the generated cases.  As for overhead, the debug build of this test took 1.6s on my laptop.

Couldn't you just take the previously crashing ones, you find important, as test cases?
The overhead is not only compile and runtime, it's also while trying to understand what's happening here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135972