[PATCH] D49466: Initial implementation of -fmacro-prefix-mapand -ffile-prefix-map

[Dan McGregor via Phabricator via cfe-commits]

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