dankm added inline comments.
================ Comment at: lib/Lex/PPMacroExpansion.cpp:1460 + for (const auto &Entry : MacroPrefixMap) + if (Path.startswith(Entry.first)) + return (Twine(Entry.second) + Path.substr(Entry.first.size())).str(); ---------------- Lekensteyn wrote: > joerg wrote: > > Lekensteyn wrote: > > > dankm wrote: > > > > dankm wrote: > > > > > joerg wrote: > > > > > > This doesn't handle directory vs string prefix prefix correctly, > > > > > > does it? At the very least, it should have a test case :) > > > > > Good catch. I expect not. But on the other hand, it's exactly what > > > > > debug-prefix-map does :) > > > > > > > > > > I'll add test cases in a future review. My first goal was getting > > > > > something sort-of working. > > > > There should be a test, but apparently the debug prefix map code also > > > > does this. > > > > > > > > What do you think the correct behaviour should be? a string prefix, or > > > > a directory prefix? > > > It should be a string prefix (like GCC) > > I disagree. I consider it a bug in GCC that it is a string prefix. It's > > quite inconsistent as well. > I agree with you, it should have been a directory prefix but GCC implements > it as a string prefix although the GCC documents it as: > "-fdebug-prefix-map=old=new When compiling files residing in **directory > old**, record debugging information describing them as if the files resided > in **directory new** instead." > > If you decide to fix `-fmacro-prefix-map` to use a directory prefix match, > then the `-fdebug-prefix-map` should also be fixed for consistency. What > about implementing the (buggy) GCC-compatible behavior first and then fixing > both cases in a future patch? (I don't mind when the buggy behavior is fixed, > I just want to see this functionality moving forward.) > > Another edge case that I ran into is when using the option to drop > directories. When using `-ffile-prefix-map=/src=`, the command `cd /src && cc > /src/foo.c` would have `__FILE__` equal to `/foo.c`. As a native "fix", one > would try `-ffile-prefix-map=/src/=` which indeed produces `__FILE__` equal > to `foo.c`. > > Matching with a trailing slash however fails to correctly remap some debug > information, namely `DW_AT_comp_dir`. This contains the working directory > (`/src`) which is not matched by `/src/`. By using a proper directory prefix > match, this would be nicely fixed. Yes, I'm going to submit my code with tests, and hoist the prefix remapping (for debug-prefix-map and macro-prefix-map) into a common location. Most probably part of Path. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D49466/new/ https://reviews.llvm.org/D49466 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
