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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
