dankm added inline comments.
================ Comment at: include/clang/Lex/PreprocessorOptions.h:171 + /// A prefix map for __FILE__ and __BASEFILE__ + std::map<std::string, std::string> MacroPrefixMap; + ---------------- erichkeane wrote: > erichkeane wrote: > > It seems this can be StringRefs as well. > Did you miss this one? Or is there a good reason these cannot be stringrefs? I didn't miss it. StringRefs here don't survive. The function that adds them to the map creates temporary strings, that go away once that function ends causing StringRefs to dangle. std::string keeps copies. ================ Comment at: lib/Driver/ToolChains/Clang.cpp:616 + } else CmdArgs.push_back(Args.MakeArgString("-fdebug-prefix-map=" + Map)); ---------------- erichkeane wrote: > With the continue above, 'else' is unnecessary/against coding standard. Next diff will have that. ================ Comment at: lib/Lex/PPMacroExpansion.cpp:1457 +static std::string remapMacroPath(StringRef Path, + const std::map<std::string, ---------------- erichkeane wrote: > Did clang-format do this formatting? It looks REALLY weird... No, that's my text editor. I'll fix it. ================ Comment at: lib/Lex/PPMacroExpansion.cpp:1532 FN += PLoc.getFilename(); + // TODO remap macro prefix here + FN = remapMacroPath(FN, PPOpts->MacroPrefixMap); ---------------- erichkeane wrote: > First, comments end in a period. Second, isn't that what the next line does? Yes, old comment is old ;) ================ 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(); ---------------- 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? ================ Comment at: lib/Lex/PPMacroExpansion.cpp:1528 // Escape this filename. Turn '\' -> '\\' '"' -> '\"' - SmallString<128> FN; + std::string FN; if (PLoc.isValid()) { ---------------- erichkeane wrote: > dankm wrote: > > erichkeane wrote: > > > This change shouldn't be necessary, SmallString is still likely the right > > > answer here. > > I tried that long ago. It didn't work, I don't remember exactly why. But I > > agree that SmallString should be enough. I'll dig more. > Just noting to handle this before approval. Yup, with some changes to remapMacroPath SmallString works fine. Repository: rC Clang https://reviews.llvm.org/D49466 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits