[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

Wed Jul 10 14:05:11 PDT 2019

aaron.ballman added a comment.

In D63139#1579231 <https://reviews.llvm.org/D63139#1579231>, @rjmccall wrote:

> I agree that tools shouldn't be forced to deal with invalid AST that looks like valid AST.  To me that means finding ways to preserve information that (1) don't badly violate invariants and (2) are easily discoverable as invalid.

I think this makes sense.

> For `case`, which has external requirements (e.g. not being a duplicate value) not entirely dissimilar to a declaration, I think that means having an "invalid" flag; arbitrary tools already can't rely on the expression being constant-evaluable because of templates, although granted many tools might never look at template patterns.  For other things (e.g. a binary operator) maybe that means using different classes (e.g. `InvalidBinaryOperator`) so that tools looking at an apparently well-typed expression don't need to consider totally invalid possibilities.

I'm not opposed to what you're saying, but I am a bit wary. IMO, we already have too many ways an AST node can be invalid that are easily checkable but totally different from node to node (sometimes child nodes are null, sometimes you check an isInvalid() predicate, sometimes you check that a type is null, sometimes we drop the node entirely, etc). I'd love to see a more uniform way to handle invalid information within an AST that retains as much source fidelity as we can get -- like ErrorDecl, ErrorStmt, ErrorExpr, and ErrorType AST nodes (perhaps these hold a partially-valid AST node of the usual kind as a child). This not only cuts down on difficulties with understanding the Clang codebase itself, but it definitely would help 3rd party tooling, pretty printing and AST dumping, AST matching, etc.

(Not that I expect that uniform way to appear as part of this patch, or even predicate the patch on it!)

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

https://reviews.llvm.org/D63139