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

[Dan McGregor via Phabricator via cfe-commits]

dankm added inline comments.


================
Comment at: lib/Driver/ToolChains/Clang.cpp:609
 static void addDebugPrefixMapArg(const Driver &D, const ArgList &Args, ArgStringList &CmdArgs) {
+  for (const Arg *A : Args.filtered(options::OPT_ffile_prefix_map_EQ)) {
+    StringRef Map = A->getValue();
----------------
joerg wrote:
> I find it confusing that `-ffile_prefix_map` implies `-fdebug-prefix-map`. I'm not sure that is desirable in every case. It seems better to have a combined option that explicitly does both.
-ffile-prefix-map is the combined option. -fmacro-prefix-map is the preprocessor option, and -fdebug-prefix-map is the codegen option.


================
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)) {
----------------
joerg wrote:
> erichkeane wrote:
> > See advice above.
> > 
> > Additionally/alternatively, I wonder if these two functions could be trivially combined.
> Or at least the for loop could be refactored into a small helper function that takes the option name, output option and error as argument.
Good ideas. I'll look into them.


================
Comment at: lib/Driver/ToolChains/Clang.cpp:1255
+
+  addMacroPrefixMapArg(D, Args, CmdArgs);
 }
----------------
erichkeane wrote:
> Is there a good reason for this to not live with the call to addDebugPrefixMapArg?
Mostly because this is the function that adds preprocessor specific options. There's no other reason why it couldn't be done alongside addDebugPrefixMapArg in this file.


================
Comment at: lib/Frontend/CompilerInvocation.cpp:2848
 
+  for (const auto &A : Args.getAllArgValues(OPT_fmacro_prefix_map_EQ))
+  {
----------------
erichkeane wrote:
> 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.
This is here because it's a preprocessor option. This function handles those. The DebugPrefixMap handling is a codegen option.


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


================
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();
----------------
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.


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


================
Comment at: lib/Lex/Preprocessor.cpp:160
+
+  for (const auto &KV : this->PPOpts->MacroPrefixMap)
+    MacroPrefixMap[KV.first] = KV.second;
----------------
erichkeane wrote:
> I'm unconvinced that this is necessary.  ExpandBuiltinMacro is in Preprocessor, so it has access to PPOpts directly.
It has access to PPOpts, but the implementation file doesn't have a full definition of PreprocessorOptions. I could add that to this file, then this becomes redundant.


Repository:
  rC Clang

https://reviews.llvm.org/D49466