[patch] Some Path-related portability FIXME fixes

Clang Frontend
1

Hi. I'd like to contribute a bit to the clang project and I thought a
good way to start would be to search for and repair some of the FIXME
items. (This seemed a safer route initially, since I am looking at
fresh code rather than a possibly-stale bug/project list.) For my
first bit of work, I've attached two patches (one each for the llvm
and clang trees) which address a few of the llvm::sys::Path-related
portability issues.

Other that the obvious "fixes" I'd like to use this first exercise to
ensure that I have the patching mechanics and etiquette correct. To
that end, I have a few questions and observations:

1. utils/mkpatch makes patches for the llvm tree. I did not see the
moral equivalent for the tools/clang tree. Did I miss it? In the
interim, I just followed the process in utils/mkpatch to make the
clang tree patch.

2. Given (1), it seemed reasonable to make two separate patches for
changes that hit both the llvm tree and the clang tree.

3. I attached each patch as a separate attachment. Is this
acceptable, or do you prefer the patch(es) to be inline with the
e-mail text?

4. Is there a 'patching best practices" document for clang? If so, I
missed it. If not, perhaps one would be helpful to new contributors.

5. Is there a style/coding document (or suggestions) for clang? I
could infer quite a bit from reading the code base, but there were
some differences file to file. I'm fine with the "inferring" approach,
but if I've overlooked your style document, please point me to it. (I
didn't see anything in the clang Developer Documentation section.)

6. I used assert() to verify that non-NULL parameters were passed to
the new IsAbsolute functions. Is this acceptable practice for clang
or is it too paranoid?

7. There is a windows change in the patch as well, but as I am running
on linux, I could only test it a bit by hacking it temporarily into
the unix version.

8. I ran the clang "make test" checks, and they ran clean. Are there
any other existing test cases that I should be running?

Thanks.
Greg

llvm.path-portability.20090611.patch (1.87 KB)

clang.path-portability.20090611.patch (1.73 KB)

2

Hi. I'd like to contribute a bit to the clang project and I thought a
good way to start would be to search for and repair some of the FIXME
items.

Great! Welcome to the community!

Other that the obvious "fixes" I'd like to use this first exercise to
ensure that I have the patching mechanics and etiquette correct. To
that end, I have a few questions and observations:

Sounds good.

1. utils/mkpatch makes patches for the llvm tree. I did not see the
moral equivalent for the tools/clang tree. Did I miss it? In the
interim, I just followed the process in utils/mkpatch to make the
clang tree patch.

utils/mkpatch should probably work, but I'm not familiar with it. I usually just use "svn diff >& out.patch".

2. Given (1), it seemed reasonable to make two separate patches for
changes that hit both the llvm tree and the clang tree.

Yes, this is preferred. Thanks!

3. I attached each patch as a separate attachment. Is this
acceptable, or do you prefer the patch(es) to be inline with the
e-mail text?

Attachments are prefered.

4. Is there a 'patching best practices" document for clang? If so, I
missed it. If not, perhaps one would be helpful to new contributors.

Yep!
http://llvm.org/docs/DeveloperPolicy.html#patches

5. Is there a style/coding document (or suggestions) for clang? I
could infer quite a bit from reading the code base, but there were
some differences file to file. I'm fine with the "inferring" approach,
but if I've overlooked your style document, please point me to it. (I
didn't see anything in the clang Developer Documentation section.)

Clang follows the same style as llvm:
http://llvm.org/docs/CodingStandards.html

6. I used assert() to verify that non-NULL parameters were passed to
the new IsAbsolute functions. Is this acceptable practice for clang
or is it too paranoid?

Assert is great. I did change your routines to take a pointer + length. This is more idiomatic in the LLVM code base, and defines away the problem when end < start. I also renamed the functions to isAbsolute to be more consistent with the existing Path methods. We don't use a capitalization approach that is consistent across the whole source base, but staying consistent per-class is goodness.

7. There is a windows change in the patch as well, but as I am running
on linux, I could only test it a bit by hacking it temporarily into
the unix version.

Yeah, this is always a problem with hacking on libsystem :frowning:

8. I ran the clang "make test" checks, and they ran clean. Are there
any other existing test cases that I should be running?

That's it! I applied a modified version of your LLVM patch here:
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20090608/078693.html

The clang part has two hunks, the first of which I applied here:
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20090608/018168.html

The second hunk changes a smallstring to use a sys::Path equivalent. The (systemic) problem with this is that sys::Path goes to the heap when it is created. The use of SmallString avoided this in the common case when the path was less than 1024 bytes long. Since this is in a hot preprocessor path, I don't think we can just accept the cost of doing this.

Unfortunately, I don't know of a good solution for this other than rewriting the Path class (which is desirable in any case).

Thanks again for tackling this Gregory, sorry for the delay getting to it,

-Chris