[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/Lex/Preprocessor.h:155
+  /// A prefix map for __FILE__ and __BASEFILE__
+  llvm::SmallDenseMap<llvm::StringRef, llvm::StringRef> MacroPrefixMap;
+
----------------
Scrolling back up, put this implementation as close to the debug implementation as you can, they are so ridiculously related that having them far enough apart that future changes could cause them to divert is troublesome to me.


================
Comment at: include/clang/Lex/PreprocessorOptions.h:171
+  /// A prefix map for __FILE__ and __BASEFILE__
+  std::map<std::string, std::string> MacroPrefixMap;
+
----------------
It seems this can be StringRefs as well.


================
Comment at: lib/Driver/ToolChains/Clang.cpp:617
+  }
   for (const Arg *A : Args.filtered(options::OPT_fdebug_prefix_map_EQ)) {
     StringRef Map = A->getValue();
----------------
filtered can take multiple options, you should be able to not add anything here except adding OPT_ffile_prefix_map_EQ to the filtered line, plus a ternary in the Diag line.


================
Comment at: lib/Driver/ToolChains/Clang.cpp:628
+/// Add a CC1 option to specify the macro file path prefix map.
+static void addMacroPrefixMapArg(const Driver &D, const ArgList &Args, ArgStringList &CmdArgs) {
+  for (const Arg *A : Args.filtered(options::OPT_ffile_prefix_map_EQ)) {
----------------
See advice above.

Additionally/alternatively, I wonder if these two functions could be trivially combined.


================
Comment at: lib/Driver/ToolChains/Clang.cpp:1255
+
+  addMacroPrefixMapArg(D, Args, CmdArgs);
 }
----------------
Is there a good reason for this to not live with the call to addDebugPrefixMapArg?


================
Comment at: lib/Frontend/CompilerInvocation.cpp:2848
 
+  for (const auto &A : Args.getAllArgValues(OPT_fmacro_prefix_map_EQ))
+  {
----------------
Again, this is so much like the debug-prefix otpion, it should probably just be right next to it.

Additionally, we don't use curley brackets on single-line bodies.


================
Comment at: lib/Lex/PPMacroExpansion.cpp:1457
+static std::string remapMacroPath(StringRef Path,
+                           llvm::SmallDenseMap<StringRef,
+                           StringRef> &MacroPrefixMap) {
----------------
make MacroPrefixMap const.


================
Comment at: lib/Lex/PPMacroExpansion.cpp:1528
     // Escape this filename.  Turn '\' -> '\\' '"' -> '\"'
-    SmallString<128> FN;
+    std::string FN;
     if (PLoc.isValid()) {
----------------
This change shouldn't be necessary, SmallString is still likely the right answer here.


================
Comment at: lib/Lex/Preprocessor.cpp:160
+
+  for (const auto &KV : this->PPOpts->MacroPrefixMap)
+    MacroPrefixMap[KV.first] = KV.second;
----------------
I'm unconvinced that this is necessary.  ExpandBuiltinMacro is in Preprocessor, so it has access to PPOpts directly.


Repository:
  rC Clang

https://reviews.llvm.org/D49466