modularize patch - request for review

[Thompson, John]

Edwin,

Thanks for the feed back.  Will do.

-John

-----Original Message-----
From: Vane, Edwin [mailto:edwin.vane at intel.com] 
Sent: Tuesday, March 12, 2013 6:16 AM
To: Thompson, John; cfe-commits at cs.uiuc.edu
Subject: RE: modularize patch - request for review

Missed this patch being in all-day training yesterday. I wasn't even aware of module functionality so I'll be looking into that. Thanks.

My review comments:
* Could you put the public interface of the class/struct at the top of the definition and the private stuff at the bottom. Things brings attention to the interface to the class and focuses less on the private stuff users of a class don't care about.
* Is it possible to make use of CommonOptionsParser for the front-end options so this tool is more consistent with the other clang-tools-extra tools? You can use the command-line library to add new options (e.g. the filename with header files in it).
 
-----Original Message-----
From: cfe-commits-bounces at cs.uiuc.edu [mailto:cfe-commits-bounces at cs.uiuc.edu] On Behalf Of Thompson, John
Sent: Monday, March 11, 2013 2:36 PM
To: cfe-commits at cs.uiuc.edu
Subject: RE: modularize patch - request for review

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


_______________________________________________
cfe-commits mailing list
cfe-commits at cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits