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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
