modularize patch - request for review

Sean Silva silvas at purdue.edu
Mon Mar 11 10:03:11 PDT 2013


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




More information about the cfe-commits mailing list