This revision was automatically updated to reflect the committed changes.
Closed by commit rC333413: Fix emission of phony dependency targets when adding
extra deps (authored by dstenb, committed by ).
Repository:
rL LLVM
https://reviews.llvm.org/D44568
Files:
This revision was automatically updated to reflect the committed changes.
Closed by commit rL333413: Fix emission of phony dependency targets when adding
extra deps (authored by dstenb, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
dstenb added a comment.
In https://reviews.llvm.org/D44568#1114188, @vsapsai wrote:
> Looks good to me. Just watch the build bots in case some of them are strict
> with warnings and require `(void)AddFilename(Filename)`.
Yes, I'll keep an eye out for that.
I'll submit this shortly. Thanks
vsapsai accepted this revision.
vsapsai added a comment.
This revision is now accepted and ready to land.
Looks good to me. Just watch the build bots in case some of them are strict
with warnings and require `(void)AddFilename(Filename)`.
As for the case when the input file is also an extra
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
>
>
dstenb updated this revision to Diff 148819.
dstenb marked an inline comment as done.
dstenb added a comment.
Addressed vsapsai's comments.
https://reviews.llvm.org/D44568
Files:
lib/Frontend/DependencyFile.cpp
test/Frontend/dependency-gen-extradeps-phony.c
Index:
vsapsai added inline comments.
Comment at: lib/Frontend/DependencyFile.cpp:182-185
for (const auto : Opts.ExtraDeps) {
AddFilename(ExtraDep);
+ ++InputFileIndex;
}
This is incorrect if there are duplicates in `Opts.ExtraDeps`. Also please
dstenb added reviewers: bruno, vsapsai.
dstenb added subscribers: bruno, vsapsai.
dstenb added a comment.
@bruno, @vsapsai: I added you since you I saw that you recently reviewed,
respectively delivered, https://reviews.llvm.org/D30881. That is the only
DependencyFile commit since October;
dstenb added a comment.
Ping.
Repository:
rC Clang
https://reviews.llvm.org/D44568
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
dstenb added a comment.
Ping.
Repository:
rC Clang
https://reviews.llvm.org/D44568
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
dstenb added a comment.
A small caveat with this patch is that it does not fix the case where the input
file as also added as an extra dependency with -fdepfile-entry; however, I
reasoned that it shouldn't really be a problem in practice. I thought that it
was a good trade-off ignoring that
dstenb created this revision.
dstenb added reviewers: rsmith, pcc, krasin.
Herald added a subscriber: cfe-commits.
This commit fixes a bug where passing extra dependency entries
(using -fdepfile-entry) would result in -MP incorrectly emitting
a phony target for the input file, and no phony target
12 matches
Mail list logo
