[PATCH] D44568: Fix emission of phony dependency targets when adding extra deps

[David Stenberg via Phabricator via cfe-commits]

dstenb added inline comments.


================
Comment at: test/Frontend/dependency-gen-extradeps-phony.c:6-7
+
+// CHECK: dependency-gen-extradeps-phony.o: 1.extra 2.extra
+// CHECK-NOT: .c:
+// CHECK: 1.extra:
----------------
vsapsai wrote:
> I think it would be better to have a check
> 
>     // CHECK:   dependency-gen-extradeps-phony.c
> 
> Because you expect it to be there and it's not that simple to notice the colon in `.c:`, so it's not immediately clear how CHECK-NOT is applied here.
Do you mean replace the two checks with that? Isn't there a risk that that would match with `dependency-gen-extradeps-phony.c:`, which the not-checks would not pick up then?

I added a CHECK-NEXT check for the input file so that we match that whole dependency entry at least.


================
Comment at: test/Frontend/dependency-gen-extradeps-phony.c:9-11
+// CHECK-NOT: .c:
+// CHECK: 2.extra:
+// CHECK-NOT: .c:
----------------
vsapsai wrote:
> For these repeated CHECK-NOT please consider using `FileCheck --implicit-check-not`. In this case it's not that important as the test is small but it can still help to capture your intention more clearly.
I'll add that in the next patch (and I'll keep that in mind for future changes). Thanks!


https://reviews.llvm.org/D44568