jdenny added a comment. This patch looks like a good idea to me. Thanks.
In D81736#2090419 <https://reviews.llvm.org/D81736#2090419>, @clementval wrote: > @jdoerfert Some tests in clang relies on the position of the specific enum in > the declaration. TableGen is sorting the records so the enum is generated in > alphabetical order therefore the enum position is changed. Is it fine to > update the test with smith like `{{.*}}` instead of the specific id? > > Example of failure: Old position 9, new position 23 > > > ./llvm-project/clang/test/AST/ast-dump-openmp-target-teams-distribute-simd.c:74:16: > error: CHECK-NEXT: expected string not found in input > // CHECK-NEXT: | | | | | `-OMPCaptureKindAttr {{.*}} <<invalid sloc>> > Implicit 9 > ^ > <stdin>:51:1: note: scanning from here > | | | | | `-OMPCaptureKindAttr 0x43ee9600 <<invalid sloc>> Implicit 23 > > This seems like a case where the dumper should be printing a name instead of a number. Given that you're touching all the tests for that anyway, I wonder how hard it would be to go ahead and fix that, perhaps in a parent patch so that this patch becomes easier to review. Ignore this suggestion if it's too much of a distraction. ================ Comment at: clang/test/AST/ast-dump-openmp-target-parallel-for.c:105 // CHECK-NEXT: | | | `-FieldDecl {{.*}} <line:5:3> col:3 implicit 'int' -// CHECK-NEXT: | | | `-OMPCaptureKindAttr {{.*}} <<invalid sloc>> Implicit 9 +// CHECK-NEXT: | | | `-OMPCaptureKindAttr {{.*}} <<invalid sloc>> Implicit 23 // CHECK-NEXT: | | `-CapturedDecl {{.*}} <<invalid sloc>> <invalid sloc> nothrow ---------------- Why 23 not `{{.*}}` like the others? ================ Comment at: llvm/include/llvm/Frontend/Directive/DirectiveBase.td:22 + // + // By default, uses the name of the dialect as the only namespace. To avoid + // placing in any namespace, use "". To specify nested namespaces, use "::" ---------------- "dialect" -> "directive language" as above? ================ Comment at: llvm/test/TableGen/directive.td:11 + let clausePrefix = "TDLC_"; + let makeEnumAvailableInNamespace = 1; +} ---------------- Does it make sense to go ahead and test the case where this is 0? What about testing enableBitmaskEnumInNamespace? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81736/new/ https://reviews.llvm.org/D81736 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
