Move clang's ParseHelper into Support (llvm part)

[olista01]

Summary

This moves clang's ParseHelper class (currently used to parse comments for the -verify option) into the Support library so that it can be used elsewhere in LLVM.

[hans]

This moves clang's ParseHelper class (currently used to parse comments for the -verify option) into the Support library so that it can be used elsewhere in LLVM.

What use do you have in mind?

I'd suggest renaming the title of the patch from "Refactor clang's ParseHelper into Support " to something like "Move clang's ParseHelper ..." since you're not so much refactoring it as moving it.

ParseHelper is currently in an anonmymous namespace inside VerifyDiagnosticConsumer, that is, it's an implementation detail of that class. The name makes sense in that context: it helps with the parsing in that file, but does the name make sense externally?

include/llvm/Support/ParseHelper.h

53

Does it make sense to have all of these as public members?

[olista01]

I'm going to use this for D27514, adding similar diagnostic verifying functionality to llvm-mc.

I think the name is still accurate, it is a helper class for writing simple parsers. Do you have a suggestion for a better name?

include/llvm/Support/ParseHelper.h

53

The current user of this class uses all of these except for End, because it needs to know the current position in the string for diagnostics, and to read back parts of the string found by SearchClosingBrace.

[hans]

In D27512#616974, @olista01 wrote:

I'm going to use this for D27514, adding similar diagnostic verifying functionality to llvm-mc.

Interesting, that looks like cool functionality.

It seems unfortunate that not more code could be shared between llvm-mc and clang's -verify though. It looks like you're essientially implementing a large chunk of it again.

It would be really cool if we could build some kind of DiagnosticVerifier utility that would work for both clang and llvm-mc. And then this ParserHelper would be an implementation of that.

[olista01]

It would be really cool if we could build some kind of DiagnosticVerifier utility that would work for both clang and llvm-mc.

I did consider that, but unfortunately the clang version of this uses clang's own systems for diagnostics, source management etc, so I think this would only be able to share a small amount of code, at the cost of a lot of added complexity.

[hans]

What worries me is that we're parsing a ton of different kinds of files all over LLVM, but this ParserHelper still seems only useful for parsing -verify things. (For example, SearchClosingBrace seems very specific, and exposing End, C, and P seems like a very specialized interface.) So there's kind of mismatch between the name and what it's for.

Would it really be that hard to extract a generally useful -verify utility? Something that parses a file and digs out the expected strings and their positions? That seems like it would be really useful.

[hans]

Looking again at D27512, you're essentially re-implementing VerifyDiagnosticConsumer.cpp's ParseDirective (much of it with copy-paste). That seems really unfortunate. If we decide to change something in the -verify syntax, we'll now have multiple places to change. If there are bugs, there are multiple places to fix.

It must be possible to at least share the code that parses a comment and figures out what directives it contains, and ideally also the code that matches those directives against diagnostics.

[AndyG]

Scanning over what is being proposed here and in D27514, I have just a couple of comments.

Its seems, firstly, that some functionality of the -verify syntax is being lost in translation to the llvm-mc tool. The missing part that I noticed is the expected-no-diagnostics directive. This may be intentional, or it may be because the reason behind its inclusion may not have been apparent, which was to ensure that test-cases would not pass when -verify was selected but no diagnostics produced, and no directives were parsed. And the reason that this might happen is simply because the test-case forgot to parse a file at all -- see http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20120924/065007.html for the background that led to this being implemented in the first place.

I think this sort of thing also argues for a more holistic approach, as suggested by Hans, in bringing this feature up to the tool-wide level.

I think it would be really nice to implement a class that encapsulates both ParserHelper and ParseDirective. There are two ways then that I could see this working. One, and probably the easier, would be as a base abstract class which would step through the main syntax of the directives, deferring to methods implemented in a sub-class which would then do the actual handling and implementation. The first method of which would be bool handleDirectiveType(SourceMgr::DiagKind Kind) which in your implementation would simply store Kind for later use, but in the clang implementation, would pick the correct DirectiveList* for later use, and so on through out.

An alternative, and perhaps conceptually clearer, approach could be to implement ParserHelper/ParseDirective as an iterator object, so that the following sort of work-flow could be implemented (each VerifyDirective object holding a pre-processed directive):

for (VerifyDirective& VD : VerifyDirectiveParser(CommentText)) {
  switch (VD.getDiagnosticKind()) {
    case SourceMgr::DK_Error:
      ...
  }
  if (VD.isRegexDirective()) {
    ...
  }
  ...
}

I haven't thought through all the details of either implementation, for example how to handle syntax errors or the best way to handle finding diagnostics in external files, but maybe there is some food for thought here. Beyond that, I really don't have an major issue with your proposal, but like Hans, I think it would be nice if it were possible for more code to be shared between the two (current) implementations.

[EricWF]

Resigning as a reviewer since I don't feel comfortable reviewing generic LLVM patches.

include/llvm/Support/ParseHelper.h

+//===- ParseHelper.h - Simple parser helper class ---------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This file declares the ParseHelper class, intended to help implement simple
+// parsers.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_SUPPORT_PARSEHELPER_H
+#define LLVM_SUPPORT_PARSEHELPER_H
+
+#include "llvm/ADT/StringRef.h"
+
+namespace llvm {
+
+class ParseHelper
+{
+public:
+  ParseHelper(StringRef S)
+    : Begin(S.begin()), End(S.end()), C(Begin), P(Begin), PEnd(nullptr) {}
+
+  // Return true if string literal is next.
+  bool Next(StringRef S);
+
+  // Return true if number is next.
+  // Output N only if number is next.
+  bool Next(unsigned &N);
+
+  // Return true if string literal is found.
+  // When true, P marks begin-position of S in content.
+  bool Search(StringRef S, bool EnsureStartOfWord = false);
+
+  // Return true if a CloseBrace that closes the OpenBrace at the current nest
+  // level is found. When true, P marks begin-position of CloseBrace.
+  bool SearchClosingBrace(StringRef OpenBrace, StringRef CloseBrace);
+
+  // Advance 1-past previous next/search.
+  // Behavior is undefined if previous next/search failed.
+  bool Advance();
+
+  // Skip zero or more whitespace.
+  void SkipWhitespace();
+
+  // Return true if EOF reached.
+  bool Done();
+
+  const char * const Begin; // beginning of expected content
+  const char * const End;   // end of expected content (1-past)
+  const char *C;            // position of next char in content
+  const char *P;
+
+private:
+  const char *PEnd; // previous next/search subject end (1-past)
+};
+
+} // namespace llvm
+
+#endif // LLVM_SUPPORT_PARSEHELPER_H

lib/Support/CMakeLists.txt

add_llvm_library(LLVMSupport
   Locale.cpp
   LockFileManager.cpp
   ManagedStatic.cpp
   MathExtras.cpp
   MemoryBuffer.cpp
   MD5.cpp
   NativeFormatting.cpp
   Options.cpp
+  ParseHelper.cpp
   PluginLoader.cpp
   PrettyStackTrace.cpp
   RandomNumberGenerator.cpp
   Regex.cpp
   ScaledNumber.cpp
   ScopedPrinter.cpp
   SHA1.cpp
   SmallPtrSet.cpp

lib/Support/ParseHelper.cpp

+//===- ParseHelper.cpp - Simple parser helper class -------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This file declares the ParseHelper class, intended to help implement simple
+// parsers.
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/Support/ParseHelper.h"
+
+using namespace llvm;
+
+static bool isWhitespace(const char C) {
+  switch (C) {
+  case ' ': case '\t': case '\f':
+  case '\v': case '\n': case '\r':
+    return true;
+  default:
+    return false;
+  }
+}
+
+// Return true if string literal is next.
+bool ParseHelper::Next(StringRef S) {
+  P = C;
+  PEnd = C + S.size();
+  if (PEnd > End)
+    return false;
+  return !memcmp(P, S.data(), S.size());
+}
+
+// Return true if number is next.
+// Output N only if number is next.
+bool ParseHelper::Next(unsigned &N) {
+  unsigned TMP = 0;
+  P = C;
+  for (; P < End && P[0] >= '0' && P[0] <= '9'; ++P) {
+    TMP *= 10;
+    TMP += P[0] - '0';
+  }
+  if (P == C)
+    return false;
+  PEnd = P;
+  N = TMP;
+  return true;
+}
+
+// Return true if string literal is found.
+// When true, P marks begin-position of S in content.
+bool ParseHelper::Search(StringRef S, bool EnsureStartOfWord) {
+  do {
+    P = std::search(C, End, S.begin(), S.end());
+    PEnd = P + S.size();
+    if (P == End)
+      break;
+    if (!EnsureStartOfWord
+          // Check if string literal starts a new word.
+          || P == Begin || isWhitespace(P[-1])
+          // Or it could be preceded by the start of a comment.
+          || (P > (Begin + 1) && (P[-1] == '/' || P[-1] == '*')
+                              &&  P[-2] == '/'))
+      return true;
+    // Otherwise, skip and search again.
+  } while (Advance());
+  return false;
+}
+
+// Return true if a CloseBrace that closes the OpenBrace at the current nest
+// level is found. When true, P marks begin-position of CloseBrace.
+bool ParseHelper::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;
+}
+
+// Advance 1-past previous next/search.
+// Behavior is undefined if previous next/search failed.
+bool ParseHelper::Advance() {
+  C = PEnd;
+  return C < End;
+}
+
+// Skip zero or more whitespace.
+void ParseHelper::SkipWhitespace() {
+  for (; C < End && isWhitespace(*C); ++C)
+    ;
+}
+
+// Return true if EOF reached.
+bool ParseHelper::Done() {
+  return !(C < End);
+}