Page MenuHomePhabricator

Fix hardcoded path seperator in two code locations
ClosedPublic

Authored by yaron.keren on May 9 2014, 2:32 AM.

Details

Summary

Earlier duscussion:
https://www.mail-archive.com/cfe-commits@cs.uiuc.edu/msg92891.html

Most of LLVM code uses the llvm::sys::path::append function to concatenate paths and will get the correct native slash. In some code locations it seems that direct string concatenation was used, possibly because the code writer knows there are no extra path sepeartors to deal with, so string concat was more efficient than append.

In the locations below, the path seperator is hardcoded as '/'. Fixed by exposing the native seperator from llvm::sys:;path and using it.

As an alternative, the append function could be used but it's more code and less efficient.

Diff Detail

Event Timeline

yaron.keren updated this revision to Diff 9239.May 9 2014, 2:32 AM
yaron.keren retitled this revision from to Fix hardcoded path seperator in two code locations.
yaron.keren updated this object.
yaron.keren edited the test plan for this revision. (Show Details)
yaron.keren added reviewers: echristo, rnk, chapuni.
yaron.keren added a subscriber: Unknown Object (MLST).
rnk accepted this revision.May 9 2014, 9:58 AM
rnk edited edge metadata.

LGTM

Let's be consistent with what append does. If users want to do cross-compiling, we'll need to thread a flag down to fs::append() so it also does the right thing.

include/llvm/Support/Path.h
301

This should return a StringRef.

This revision is now accepted and ready to land.May 9 2014, 9:58 AM
yaron.keren updated this revision to Diff 9270.May 9 2014, 12:32 PM
yaron.keren edited edge metadata.

With StringRef.

rnk added inline comments.May 9 2014, 1:43 PM
lib/Support/Path.cpp
573

We don't need this global data. It will probably require a dynamic initializer, which is lame. Clang is smart enough to optimize out strlen if you just return preferred_separator directly:

$ cat t.cpp
extern "C" int strlen(const char *s);
struct StringRef {
  const char *p;
  size_t len;
  StringRef(const char *s) : p(s), len(strlen(s)) {}
};
static const char preferred_sep[] = { '/', '\0' };
StringRef getPrefSep() {
  return StringRef(preferred_sep);
}

$ clang -c t.cpp -O2 -S -o - --target=x86_64-linux -fPIC | grep '^\s[^\.]'
        leaq    _ZL13preferred_sep(%rip), %rax
        movl    $1, %edx
        retq
yaron.keren updated this revision to Diff 9276.May 10 2014, 2:03 AM

Static StringRef removed.

rnk added a comment.May 10 2014, 4:00 PM

Feel free to commit, those where just nits.

yaron.keren closed this revision.May 16 2014, 6:24 AM

Committed revision 208980.