tahonermann added a comment. > @tahonermann are all your comments addressed at this point?
Apologies for the late response; I was on vacation last week. No. I'm still skeptical that there is ever a desire to expand substitute drives. ================ Comment at: clang/test/Lexer/case-insensitive-include-win.c:5-9 +// Note: We must use the real path here, because the logic to detect case +// mismatch relies on resolving the real path and checking that casing differs. +// If we use %t and we are on a substitute drive S: mapping to C:\subst, +// then we will compare "S:\test.dir\FOO.h" to "C:\subst\test.dir\foo.h" +// and avoid emitting the diagnostic because the structure is different. ---------------- MrTrillian wrote: > tahonermann wrote: > > MrTrillian wrote: > > > tahonermann wrote: > > > > Hmm, is it really desirable or necessary that the case mismatch logic > > > > resolve substituted drives? I wouldn't think so. This seems like > > > > another case where our common canonicalization would be desired. > > > I think it is necessary as I don't see how else we can retrieve the > > > original casing of the file path. Merely making the path absolute would > > > not do that. Searching for solutions on the internet finds ideas that are > > > worse like converting to a short path then back to a long path or > > > rebuilding the path one component at a time with a series directory > > > listing requests. > > The warning this test checks for is diagnosed in > > `Preprocessor::HandleHeaderIncludeOrImport()`; search for > > `pp_nonportable_path` and/or `pp_nonportable_system_path`. I believe this > > warning will be issued if any component of the path does not match the case > > of the include directive. Since the file name differs in case, unless there > > is a bug in handling of the file name, this test isn't sensitive to case > > mismatches in `%t`. > > > > From a user point of view, resolving a substitute drive doesn't seem > > desirableto me with respect to determining whether a non-portable path is > > being used; I don't think the behavior should be dependent on whether a > > substitute drive is being used or what its target is. > @tahonermann I think the code is working by design and it would be an > unrelated change to modify its logic. See `trySimplifyPath` in > `PPDirectives.cpp`: > > ``` > // Below is a best-effort to handle ".." in paths. It is admittedly > // not 100% correct in the presence of symlinks. > > // If these path components differ by more than just case, then we > // may be looking at symlinked paths. Bail on this diagnostic to avoid > // noisy false positives. > ``` > > The test was previously implicitly requiring getting the realpath, and it > worked because `lit.py`'s logic of doing that. Now that requirement is > explicit in the test. I'm still not following here. Are you saying that `trySimplifyPath()` will replace substitute drives? If so, that doesn't sound desirable and I would expect it to be problematic for your use case. I think we should follow this up ... somewhere (now that this review is closed). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154130/new/ https://reviews.llvm.org/D154130 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
