[cfe-dev] [patch] Some Path-related portability FIXME fixes

[Chris Lattner]

On Jun 11, 2009, at 7:39 AM, Gregory Curfman wrote:
> 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 :(

> 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