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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
