[PATCH] D30882: Add a callback for __has_include and use it for dependency scanning

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai marked 3 inline comments as done.
vsapsai added inline comments.


================
Comment at: include/clang/Lex/PPCallbacks.h:263
+  /// \brief Callback invoked when a has_include directive is read.
+  virtual void HasInclude(SourceLocation Loc, const FileEntry *File) {
+  }
----------------
rsmith wrote:
> This callback seems pretty unhelpful in the case where lookup of the file failed (you just get a source location). Could we pass in the `FileName` and `IsAngled` flag too (like we do for `InclusionDirective`)? I think that's what (eg) a callback tracking anti-dependencies (for a more sophisticated build system that can handle them) would want.
Added. Also added `FileType` so we can ignore system headers for `-MMD`. And tweaked doxygen comment to be more consistent with other callbacks.


================
Comment at: test/Frontend/dependency-gen-has-include.c:11-15
+// RUN: %clang -MD -MF %t.dir/file.deps %s -fsyntax-only -I %t.dir
+// RUN: FileCheck -input-file=%t.dir/file.deps %s
+// CHECK: dependency-gen-has-include.o
+// CHECK: dependency-gen-has-include.c
+// CHECK: a/header.h
----------------
dexonsmith wrote:
> This covers quoted includes when there is a single `-I`.  I think there are a couple of other cases worth testing:
> - Multiple `-I` arguments: all should be listed.
> - Angle includes: all `-isystem` should be listed too.
> - Also `-F` and `-iframework`.
I've added a test with system headers. Overall, my testing strategy was to test all cases for the following dimensions:
* file exists/file doesn't exist;
* file is user/system header;
* `__has_include`/`__has_include_next` is used.

But I don't test the full matrix as I don't think it provides enough value.


================
Comment at: test/Frontend/dependency-gen-has-include.c:17
+#if __has_include("a/header.h")
+#endif
----------------
bruno wrote:
> Can you also add a testcase for `__has_include_next`?
Added.


https://reviews.llvm.org/D30882