joerg 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();
----------------
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.
================
Comment at: lib/Driver/ToolChains/Clang.cpp:612
+ if (Map.find('=') == StringRef::npos)
+ D.Diag(diag::err_drv_invalid_argument_to_fdebug_prefix_map) << Map;
+ else
----------------
I'd prefer the bailout style here, i.e. `if (...) { D.diag(...); continue }`
================
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)) {
----------------
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.
================
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();
----------------
This doesn't handle directory vs string prefix prefix correctly, does it? At
the very least, it should have a test case :)
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
