[PATCH] D58094: Fix -Wnonportable-include-path suppression for header maps with absolute paths.

[Volodymyr Sapsai via Phabricator via cfe-commits]

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


================
Comment at: clang/lib/Lex/HeaderSearch.cpp:888
     bool InUserSpecifiedSystemFramework = false;
-    bool HasBeenMapped = false;
+    bool CurrentInHeaderMap = false;
     bool IsFrameworkFoundInDir = false;
----------------
dexonsmith wrote:
> Why not name this the same way as the parameter?  (`IsInHeaderMap`)
Wanted to emphasize it applies only to the current search directory. But I think what matters is to distinguish between being mentioned in header map and changing the search name. So I'll drop "Current" and suggestions for better naming are welcome.


================
Comment at: clang/lib/Lex/HeaderSearch.cpp:896-897
+      assert(CurrentInHeaderMap && "MappedName should come from a header map");
       CacheLookup.MappedName =
-          copyString(Filename, LookupFileCache.getAllocator());
-      if (IsMapped)
-        *IsMapped = true;
+          copyString(MappedName, LookupFileCache.getAllocator());
+      HasBeenMapped = true;
----------------
dexonsmith wrote:
> It's not obvious to me why this changed from `Filename` to `MappedName`.  Can you explain it?
Technically it doesn't matter because in this branch they are equal (would an assertion be useful?). But with my change `IsInHeaderMap` and `MappedName` can be different for absolute paths. So I think it is more natural

```lang=c++
if (!MappedName.empty())
  CacheLookup.MappedName = copyString(MappedName);
```

than

```lang=c++
if (!MappedName.empty())
  CacheLookup.MappedName = copyString(Filename);
```

In the second case I need to check that `LookupFile` can modify `Filename`. But I don't have a strong opinion about it.


================
Comment at: clang/lib/Lex/HeaderSearch.cpp:898
+          copyString(MappedName, LookupFileCache.getAllocator());
+      HasBeenMapped = true;
     }
----------------
dexonsmith wrote:
> Do we need `HasBeenMapped` outside of the loop, or can you just use the loop-local variable?
Yes. Let me add a test case and a comment explaining it.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58094/new/

https://reviews.llvm.org/D58094