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

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai marked 2 inline comments as done.
vsapsai added a comment.

Thanks for the review.



================
Comment at: clang/lib/Lex/HeaderSearch.cpp:892-902
+        IsInHeaderMap, MappedName);
+    if (!MappedName.empty()) {
+      assert(IsInHeaderMap && "MappedName should come from a header map");
       CacheLookup.MappedName =
-          copyString(Filename, LookupFileCache.getAllocator());
-      if (IsMapped)
-        *IsMapped = true;
+          copyString(MappedName, LookupFileCache.getAllocator());
     }
+    if (IsMapped)
----------------
dexonsmith wrote:
> I wonder if this would be easier to follow if you refactored like this:
> 
> ```
> if (!MappedName.empty()) {
>   // other logic.
>   if (IsMapped)
>     *IsMapped = true;
> } else if (IsInHeaderMap && File) {
>   if (IsMapped)
>     *IsMapped = true;
> }
> ```
> 
> but maybe my aesthetics are being thrown off by all the intervening comments in Phab.  I'll leave it up to you.
I've tried the suggested approach but don't find it better. It is a personal preference but I like how `*IsMapped |= ...` conveys the value is an "aggregate" of the previous file lookups.


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

https://reviews.llvm.org/D58094