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
