[PATCH] Change semantics of regex expectations in the diagnostic verifier

[Hans Wennborg]

  Thanks for the review! New patch coming up.


================
Comment at: test/Sema/thread-specifier.c:24
@@ -23,3 +23,3 @@
 #ifdef __cplusplus
-// expected-error-re at -2 {{'(__thread|_Thread_local|thread_local)' is only allowed on variable declarations}}
+// expected-error-re at -2 {{'{{(__thread|_Thread_local|thread_local)}}' is only allowed on variable declarations}}
 #else
----------------
Dmitri Gribenko wrote:
> Parens are probably not needed anymore (here and in other similar places).  If they are needed, I would actually expect them to be added internally automatically.
They are currently needed. The way the code works is that it pastes all the regex and non-regex parts of the directive together into one big regex.

Adding parens around each regex part automatically sounds like a good idea, though. I'll do that.

================
Comment at: include/clang/Frontend/VerifyDiagnosticConsumer.h:124-127
@@ -122,6 +123,6 @@
 ///
-///   // expected-error-re {{variable has has type 'struct .'}}
-///   // expected-error-re {{variable has has type 'struct .*'}}
-///   // expected-error-re {{variable has has type 'struct (.*)'}}
-///   // expected-error-re {{variable has has type 'struct[[:space:]](.*)'}}
+///   // expected-error-re {{variable has has type 'struct {{.}}'}}
+///   // expected-error-re {{variable has has type 'struct {{.*}}'}}
+///   // expected-error-re {{variable has has type 'struct {{(.*)}}'}}
+///   // expected-error-re {{variable has has type 'struct{{[[:space:]](.*)}}'}}
 /// \endcode
----------------
Richard Smith wrote:
> "has has"? =)
Done.

================
Comment at: lib/Frontend/VerifyDiagnosticConsumer.cpp:254-274
@@ +253,23 @@
+  // level is found. When true, P marks begin-position of CloseBrace.
+  bool SearchClosingBrace(StringRef OpenBrace, StringRef CloseBrace) {
+    unsigned Depth = 1;
+    P = C;
+    while (P < End) {
+      StringRef S(P, End - P);
+      if (S.startswith(OpenBrace)) {
+        ++Depth;
+        P += OpenBrace.size();
+      } else if (S.startswith(CloseBrace)) {
+        --Depth;
+        if (Depth == 0) {
+          PEnd = P + CloseBrace.size();
+          return true;
+        }
+        P += CloseBrace.size();
+      } else {
+        ++P;
+      }
+    }
+    return false;
+  }
+
----------------
Richard Smith wrote:
> I don't think this does the right thing for cases like "\{{" and "\}}".
These directives don't really support any escaping (except for \n apparently, and it will get \\n "wrong"). I'd rather hold off adding support for escaping if you don't think this is important.

================
Comment at: lib/Frontend/VerifyDiagnosticConsumer.cpp:848
@@ +847,3 @@
+// Add the characters from FixedStr to RegexStr, escaping as needed.  This
+// avoids "leaning toothpicks" in common patterns.
+static void AddFixedStringToRegEx(StringRef FixedStr, std::string &RegexStr) {
----------------
Richard Smith wrote:
> Took me a second to figure out what you meant here. Maybe instead say this avoids the need for backslash-escaping?
Done.

================
Comment at: lib/Frontend/VerifyDiagnosticConsumer.cpp:880
@@ -825,4 +879,3 @@
                              unsigned Min, unsigned Max) {
-  if (RegexKind)
-    return new RegexDirective(DirectiveLoc, DiagnosticLoc, Text, Min, Max);
-  return new StandardDirective(DirectiveLoc, DiagnosticLoc, Text, Min, Max);
+  if (!RegexKind || Text.find("{{") == StringRef::npos)
+    return new StandardDirective(DirectiveLoc, DiagnosticLoc, Text, Min, Max);
----------------
Richard Smith wrote:
> Should we diagnose a `-re` match that doesn't contain a `'{{'`? Either that, or drop the `-re` form entirely and always allow regexps?
My first idea was actually to drop the -re forms, as you say. However, double curly braces come up in diagnostic output that we want to match, which seemed like a good reason not to do this. For example  test\SemaCXX\constexpr-printing.cpp:41.

I'll add a diagnostic.

================
Comment at: test/Analysis/analyzer-stats.c:5
@@ -4,3 +4,3 @@
 
-int test() { // expected-warning-re{{test -> Total CFGBlocks: [0-9]+ \| Unreachable CFGBlocks: 0 \| Exhausted Block: no \| Empty WorkList: yes}}
+int test() { // expected-warning-re{{{{test -> Total CFGBlocks: [0-9]+ \| Unreachable CFGBlocks: 0 \| Exhausted Block: no \| Empty WorkList: yes}}}}
   int a = 1;
----------------
Richard Smith wrote:
> `{{{{`? :(
> 
> Can we write this as:
> 
>   // expected-warning-re{{test -> Total CFGBlocks: {{[0-9]+}} | Unreachable CFGBlocks: 0 | Exhausted Block: no | Empty WorkList: yes}}
> 
> ?
Yes! Done.

================
Comment at: test/Misc/verify.c:13-14
@@ +12,3 @@
+struct s r3; // expected-error-re {{tentative definition has type '{{(.*)[[:space:]]*(.*)}}' that is never completed}}
+struct s r4; // expected-error-re {{{{^tentative}}}}
+struct s r5; // expected-error-re {{{{completed$}}}}
----------------
Richard Smith wrote:
> Maybe:
> 
>   {{{{^}}tentative}}
>   {{completed{{$}}}}
Done.


http://llvm-reviews.chandlerc.com/D2388