vsapsai marked 2 inline comments as done.
vsapsai added inline comments.
================
Comment at: clang/lib/Lex/HeaderSearch.cpp:821
CurDir = nullptr;
+ bool HasBeenMapped = false;
----------------
arphaman wrote:
> NIT: It looks like `HasBeenMapped` should be always set when
> `CacheLookup.MappedName` is set as well. Would it make sense to check
> `CacheLookup.MappedName` instead to avoid divergence in the future? You could
> have a helper lambda `HasBeenMapped = [] (&) { return CacheLookup.MappedName;
> }` as well.
Good point, will change. Now that you've mentioned it, keeping `IsMapped` and
`CacheLookup.MappedName` in sync doesn't look great.
================
Comment at:
clang/test/Preprocessor/include-header-missing-in-framework-with-headermap.c:10
+
+#ifdef LATE_REMAPPING
+// Framework is found before remapping.
----------------
arphaman wrote:
> would it make sense to exercise both #ifdef in the test in the two clang
> invocations, but guard the expected-errors with the `#ifdef`? This way the
> test can ensure that the error is not emitted in the other invocation.
The tricky part is that "file not found" is a fatal error and `%clang_cc1` will
ignore subsequent errors. So we cannot test both includes in the same
invocation. It is possible to test all combinations by adding 2 more clang
invocations but those extra cases aren't very useful.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61707/new/
https://reviews.llvm.org/D61707
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
