modularize patch - request for review

[Thompson, John]

Sean,

Thanks for the excellent feedback.

>Regarding documentation, we currently have no documentation about how to actually set up and use the experimental modules functionality. It is my understanding that you are working on setting up modules, and it would be great if you could share how to compile a "hello world" of modules. Currently there is a complete lack (AFAIK) of concrete information about how to use modules (e.g. what command line options to use?), yet folks are *dying* to be able to try out the new module functionality and some documentation regarding how to do that. Of course, more people banging on it will bring more bug reports, suggestions, and overall improvements.

I'm just working on improving the modularize tool, which is used to help users prepare a set of headers for module'-ability.  Doug will probably have to kick-start the documentation and samples on the actual module functionality when it is ready, as I don't know myself at present.  I'm available if Doug want's help with it.

> I'm not sure why Doug initially used stdio.h for IO instead of raw_ostream, but for consistency with the rest of the codebase it would probably be best to switch over to using raw_osteam.

Sounds reasonable.  I'll look into it.

>Instead of directly using getcwd(), can you use llvm::sys::fs::current_path()?
><http://llvm.org/docs/doxygen/html/namespacellvm_1_1sys_1_1fs.html#ac7b1d477269428b188cef6585fe6fdf5>.

Ditto.

>I think that should allow removing the platform-specific includes and #ifdefs, which are generally frowned upon <http://llvm.org/docs/SystemLibrary.html#don-t-include-system-headers>.

Ditto.

>> Note that I wasn't sure if I should add something to the extra/doc directory, but I will if you indicate such.

>I think we can wait for the tool to mature a bit before doing this.
>For now, I think it would be sufficient if you beefed up the file-level comment with a micro-manpage detailing the arguments and their meaning.

Will do.  Funny, I sort of remember seeing a readme.txt file or something about it, but I couldn't find it.

>One thing you may want to consider is how to test this tool. It would be nice if the initial check-in also laid the foundation for testing in clang-tools-extra/test/, so that we can make sure things don't regress. It would also serve >as a basic form of developer documentation, which is appropriate at the moment since this is currently not a user-facing tool.

Good idea.  Will do.

Thanks again.

-John

-----Original Message-----
From: Sean Silva [mailto:silvas at purdue.edu] 
Sent: Monday, March 11, 2013 10:03 AM
To: Thompson, John
Cc: cfe-commits at cs.uiuc.edu
Subject: Re: modularize patch - request for review

Hi John, this looks like a great start.

Regarding documentation, we currently have no documentation about how to actually set up and use the experimental modules functionality. It is my understanding that you are working on setting up modules, and it would be great if you could share how to compile a "hello world" of modules. Currently there is a complete lack (AFAIK) of concrete information about how to use modules (e.g. what command line options to use?), yet folks are *dying* to be able to try out the new module functionality and some documentation regarding how to do that. Of course, more people banging on it will bring more bug reports, suggestions, and overall improvements.

I'm not sure why Doug initially used stdio.h for IO instead of raw_ostream, but for consistency with the rest of the codebase it would probably be best to switch over to using raw_osteam.

Instead of directly using getcwd(), can you use llvm::sys::fs::current_path()?
<http://llvm.org/docs/doxygen/html/namespacellvm_1_1sys_1_1fs.html#ac7b1d477269428b188cef6585fe6fdf5>.
I think that should allow removing the platform-specific includes and #ifdefs, which are generally frowned upon <http://llvm.org/docs/SystemLibrary.html#don-t-include-system-headers>.

> Note that I wasn't sure if I should add something to the extra/doc directory, but I will if you indicate such.

I think we can wait for the tool to mature a bit before doing this.
For now, I think it would be sufficient if you beefed up the file-level comment with a micro-manpage detailing the arguments and their meaning.

One thing you may want to consider is how to test this tool. It would be nice if the initial check-in also laid the foundation for testing in clang-tools-extra/test/, so that we can make sure things don't regress. It would also serve as a basic form of developer documentation, which is appropriate at the moment since this is currently not a user-facing tool.

-- Sean Silva