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

Volodymyr Sapsai via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu May 24 15:28:02 PDT 2018

vsapsai added inline comments.

Comment at: lib/Frontend/DependencyFile.cpp:182-185
     for (const auto &ExtraDep : Opts.ExtraDeps) {
+      ++InputFileIndex;
This is incorrect if there are duplicates in `Opts.ExtraDeps`. Also please update the test to have duplicate extra dependencies.

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:
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.

Comment at: test/Frontend/dependency-gen-extradeps-phony.c:9-11
+// CHECK-NOT: .c:
+// CHECK: 2.extra:
+// CHECK-NOT: .c:
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.

  rC Clang


More information about the cfe-commits mailing list