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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
Comment at: include/clang/Basic/DiagnosticDriverKinds.td:118
   "invalid deployment target for -stdlib=libc++ (requires %0 or later)">;
+def err_drv_invalid_argument_to_fmacro_prefix_map : Error<
+  "invalid argument '%0' to -fmacro-prefix-map">;
----------------
Since these are otherwise identical, perhaps a %select{...|...} for the flag name?


================
Comment at: include/clang/Lex/PreprocessorOptions.h:171
+  /// A prefix map for __FILE__ and __BASEFILE__
+  std::map<std::string, std::string> MacroPrefixMap;
+
----------------
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?


================
Comment at: lib/Driver/ToolChains/Clang.cpp:616
+    }
     else
       CmdArgs.push_back(Args.MakeArgString("-fdebug-prefix-map=" + Map));
----------------
With the continue above, 'else' is unnecessary/against coding standard.


================
Comment at: lib/Driver/ToolChains/Clang.cpp:631
+    }
+    else
+      CmdArgs.push_back(Args.MakeArgString("-fmacro-prefix-map=" + Map));
----------------
See above.


================
Comment at: lib/Lex/PPMacroExpansion.cpp:1457
 
+static std::string remapMacroPath(StringRef Path,
+                           const std::map<std::string,
----------------
Did clang-format do this formatting?  It looks REALLY weird...


================
Comment at: lib/Lex/PPMacroExpansion.cpp:1532
       FN += PLoc.getFilename();
+      // TODO remap macro prefix here
+      FN = remapMacroPath(FN, PPOpts->MacroPrefixMap);
----------------
First, comments end in a period.  Second, isn't that what the next line does?


================
Comment at: lib/Lex/PPMacroExpansion.cpp:1528
     // Escape this filename.  Turn '\' -> '\\' '"' -> '\"'
-    SmallString<128> FN;
+    std::string FN;
     if (PLoc.isValid()) {
----------------
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.


Repository:
  rC Clang

https://reviews.llvm.org/D49466