[PATCH] D61707: [Preprocessor] Fix crash emitting note with framework location for "file not found" error.

[Volodymyr Sapsai via Phabricator via cfe-commits]

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