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

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added inline comments.


================
Comment at: lib/Frontend/DependencyFile.cpp:182-185
     for (const auto &ExtraDep : Opts.ExtraDeps) {
       AddFilename(ExtraDep);
+      ++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.


Repository:
  rC Clang

https://reviews.llvm.org/D44568