jansvoboda11 accepted this revision. jansvoboda11 added a comment. In D156749#4562469 <https://reviews.llvm.org/D156749#4562469>, @vsapsai wrote:
> In D156749#4561803 <https://reviews.llvm.org/D156749#4561803>, @jansvoboda11 > wrote: > >> My suggestion is to use the actual real on-disk path. Not >> `FileEntryRef::getName()`, but something that always behaves as if >> `use-external-name` was set to `true`. I believe this would handle your >> VFS/VFS-use-external-name-true/VFS-use-external-name-false problem. It would >> also handle another pitfall: two compilations with distinct VFS overlays >> that redirect two different as-requested module map paths into the same >> on-disk path. > > Do you suggest doing it for the hashing or for ASTWriter or both? We are > already doing some module map path canonicalization (added a comment in > corresponding place) but it's not pure on-disk path, it takes into account > VFS. I mainly meant in `<hash2>`. And then make the PCM files VFS-agnostic, which would probably require us to do the same thing for all the paths we serialize there. But I don't know how feasible that is. Besides, the scanner relies on the virtual paths in PCM files. I think your solution is the most pragmatic. If you're confident this doesn't break anything internally, I say go for it. But I think it's good to be aware of the pitfall I mentioned, and make sure the build system doesn't trigger that. ================ Comment at: clang/lib/Serialization/ASTWriter.cpp:1330 AddPath(WritingModule->PresumedModuleMapFile.empty() - ? Map.getModuleMapFileForUniquing(WritingModule)->getName() + ? Map.getModuleMapFileForUniquing(WritingModule) + ->getNameAsRequested() ---------------- Can we canonicalize this also? It'd be useful in the scanner. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156749/new/ https://reviews.llvm.org/D156749 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
