keith added a comment. In D111457#3082398 <https://reviews.llvm.org/D111457#3082398>, @mstorsjo wrote:
> In D111457#3073726 <https://reviews.llvm.org/D111457#3073726>, @keith wrote: > >> In D111457#3066508 <https://reviews.llvm.org/D111457#3066508>, @mstorsjo >> wrote: >> >>> Wouldn't this one also be solved pretty much the same, but differently, by >>> changing `if (llvm::sys::path::is_absolute(RemappedFile)) {` into >>> `is_absolute_gnu`? >> >> I think it //could// but also users could still remap to native window's >> paths, so likely we'd want this test as well I think either way? > > I guess this might be a good addition as a new test, yeah, but I think it > would be good to keep this test as is too, and change the code to use > `is_absolute_gnu` (and fix up the test reference here to expect an empty > directory in the output). > >>> Since we're remapping debug paths, it's plausible that the target path can >>> be a different style (when cross compiling, where debug prefix remapping is >>> kinda important), and then it's probably good to use a more lax definition >>> of whether a path is absolute. (Alternatively, we maybe should try to >>> detect the kind of path used and use the appropriate style as argument to >>> `is_absolute`, but I don't think that necessarily helps here.) >> >> Good point here, I could definitely see wanting to support the entire matrix >> of host windows paths vs not, and target windows paths vs not, but I think >> that would require significantly more changes for other places that call >> `llvm::sys::path::*` APIs and also use the default `native` argument (I'm >> not sure how difficult this would be, but it would require a bit of auditing) > > I would expect that to mostly work so far, except for corner cases like > these. Auditing probably doesn't hurt if one wants to spend the effort, > otherwise I'd just expect it to work and try it out and see if one runs into > any issues somewhere, if someone has such a usecase. Yep this makes sense. I would rather not scope that into this if that's ok. Since this test was already invalid and broken (actual fix in https://reviews.llvm.org/D111579) I would rather land these and then take that on to unblock my original use case (https://reviews.llvm.org/D111587) if I can find some more time Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111457/new/ https://reviews.llvm.org/D111457 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
