modularize, modularize tests - please review

[Thompson, John]

Yikes, I forgot the file again.

From: cfe-commits-bounces at cs.uiuc.edu [mailto:cfe-commits-bounces at cs.uiuc.edu] On Behalf Of Thompson, John
Sent: Monday, March 25, 2013 4:32 PM
To: Vane, Edwin; Sean Silva
Cc: cfe-commits at cs.uiuc.edu
Subject: modularize, modularize tests - please review

Sorry, there was a mistake with the later patch, having left in an experiment with the path given to Compilations.reset.  This puts it back, plus the other changes mentioned below.

May I check it in?

Thanks.

-John

From: cfe-commits-bounces at cs.uiuc.edu<mailto:cfe-commits-bounces at cs.uiuc.edu> [mailto:cfe-commits-bounces at cs.uiuc.edu] On Behalf Of Thompson, John
Sent: Friday, March 22, 2013 4:22 PM
To: Vane, Edwin; Sean Silva
Cc: cfe-commits at cs.uiuc.edu<mailto:cfe-commits at cs.uiuc.edu>
Subject: RE: modularize tests - please review

This time, with the patch.

From: Thompson, John
Sent: Friday, March 22, 2013 3:52 PM
To: 'Vane, Edwin'; Sean Silva
Cc: cfe-commits at cs.uiuc.edu<mailto:cfe-commits at cs.uiuc.edu>
Subject: RE: modularize tests - please review

Thanks again for the feedback.

In the enclosed patch I've reworked how modularize handles arguments, switching it to use the CommandLine.h API, and adding the file path prefixing we discussed.  I tried using CommonOptionParser.h but the CommandLine.h  parser seemed to be unhappy about the ConsumeAfter option I added, probably because of the positional argument that CommonOptionParser defines, so I think I did the next best thing, which is to use the underlying CommanLine.h stuff.  Are there other standard tool options I should be handling?

The above change allowed me to remove the "cd" from the tests.

I'm a newbie with respect to the types and string handling in LLVM, so perhaps you could check if I did reasonable things with the paths and so forth in the Modularize.cpp changes.

I tested this with a separate build tree on both Windows and Linux.

Thanks.

-John

From: Vane, Edwin [mailto:edwin.vane at intel.com]
Sent: Friday, March 22, 2013 1:07 PM
To: Sean Silva; Thompson, John
Cc: cfe-commits at cs.uiuc.edu<mailto:cfe-commits at cs.uiuc.edu>
Subject: RE: modularize tests - please review

Sorry it took me so long to get to this review. I agree with Sean regarding how the headers should be located relative to the header-list file. Using clang::HeaderSearch is probably overkill.

From: cfe-commits-bounces at cs.uiuc.edu<mailto:cfe-commits-bounces at cs.uiuc.edu> [mailto:cfe-commits-bounces at cs.uiuc.edu] On Behalf Of Sean Silva
Sent: Thursday, March 21, 2013 6:30 PM
To: Thompson, John
Cc: cfe-commits at cs.uiuc.edu<mailto:cfe-commits at cs.uiuc.edu>
Subject: Re: modularize tests - please review



On Wed, Mar 20, 2013 at 9:26 PM, Thompson, John <John_Thompson at playstation.sony.com<mailto:John_Thompson at playstation.sony.com>> wrote:
Sean,

I tried using -I to fix the path problem, but modularize doesn't use the include path when trying to read the files from the source input list.  That could be changed, but it's probably better to let it remain explicit where the sources come from.  Perhaps I could change modularize to look for the files relative to the list file, or add a separate option for specifying the directory to use for a reference, or perhaps both.


I think it makes sense to look for the files relative to the list file, with a commandline option to override that behavior.

-- Sean Silva
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130325/d2e31369/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: modularize_tests_4.patch
Type: application/octet-stream
Size: 9833 bytes
Desc: modularize_tests_4.patch
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130325/d2e31369/attachment.obj>