[PATCH] D27512: Move clang's ParseHelper into Support (llvm part)

[Andy Gibbs via Phabricator via llvm-commits]

AndyG added a comment.

Scanning over what is being proposed here and in https://reviews.llvm.org/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.


Repository:
  rL LLVM

https://reviews.llvm.org/D27512