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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to