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