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

[Peter Wu via Phabricator via llvm-commits]

Lekensteyn added a comment.

Thanks for picking this up again. I've left some nitpicks below in a quick review.

The "strict" parameter is not precisely defined, if that is fixed I think this would be ready for merge.



================
Comment at: clang/test/Driver/debug-prefix-map.c:8
+// RUN: %clang -### -ffile-prefix-map=old=new %s 2>&1 | FileCheck %s -check-prefix CHECK-DEBUG-SIMPLE
+// RUN: %clang -### -ffile-prefix-map=old=new %s 2>&1 | FileCheck %s -check-prefix CHECK-MACRO-SIMPLE
+
----------------
What about combining these two tests? The command is the same, maybe you could have a new `-check-prefix` to reduce the number of invocations? Likewise for the cases below.


================
Comment at: llvm/include/llvm/Support/Path.h:172
+/// @param style The path separator style
+/// @param strict Strict prefix path checking
+/// @result true if \a Path begins with OldPrefix
----------------
"strict checking" is ambiguous on its own. What about something like:

    If strict is true, a directory separator following \a OldPrefix will also be stripped. Otherwise, directory separators will only be matched and stripped when present in \a OldPrefix.

Or whatever semantics you would like to assign to "strict mode".


================
Comment at: llvm/include/llvm/Support/Path.h:181
+  return replace_path_prefix(Path, OldPrefix, NewPrefix, style, strict);
+}
 
----------------
Why have a variant with the parameters swapped, is it common in LLVM to have such convenience wrappers?

Why not require callers to pass `Style::native` whenever they want to modify "strict"?


================
Comment at: llvm/lib/Support/Path.cpp:512
+  if (!strict && OldPrefix.size() > OrigPath.size())
+    return false;
+
----------------
this condition is duplicated above


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D49466/new/

https://reviews.llvm.org/D49466