[PATCH] D129222: [pseudo] Implement a guard to determine function declarator.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

impl looks good!



================
Comment at: clang-tools-extra/pseudo/test/cxx/declarator-function.cpp:6
 // RUN: clang-pseudo -grammar=cxx -source=%s --print-forest | FileCheck %s
-void s(){};
-// CHECK-NOT:      simple-declaration
-// CHECK:          function-definition := decl-specifier-seq declarator
-// function-body CHECK-NOT:      simple-declaration
+void s() {};
+// CHECK:      ├─declaration-seq~function-definition := decl-specifier-seq function-declarator function-body
----------------
I'm not convinced that rewriting this test (rather than just removing the FIXME and XFAIL) is an improvement.

We're asserting a lot more stuff, but none of it is actually important. I find it tends to make it hard to find the signal among the noise of failures.


================
Comment at: clang-tools-extra/pseudo/test/cxx/declarator-function.cpp:7
+void s() {};
+// CHECK:      ├─declaration-seq~function-definition := decl-specifier-seq function-declarator function-body
+// CHECK-NEXT: │ ├─decl-specifier-seq~VOID := tok[0]
----------------
FWIW: I think that including the full contents of each line is bit unneccesary and brittle.

e.g. you can leave off the `:= ...` without any risk of matching spuriously


================
Comment at: clang-tools-extra/pseudo/test/cxx/declarator-var.cpp:6
 // RUN: clang-pseudo -grammar=cxx -source=%s --print-forest | FileCheck %s
 void (*s)(){};
 // CHECK-NOT:      function-definition
----------------
this is testing essentially the same thing as the new `int s2` case you added (though this one shows the subtle difference better IMO).

I think you should keep either one, but not both.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129222