This is an archive of the discontinued LLVM Phabricator instance.

merge module-map-checker into modularize
AbandonedPublic

Authored by jtsoftware on Feb 6 2015, 2:47 PM.

Details

Reviewers
silvas
rsmith
Summary

This is the first stop in some enhancements I'm making to modularize. This patch merges the module map coverage check from the existing module-map-checker tool into modularize. It also allows a module map file to be used as the input to modularize, for doing the usual checks modularize does, but extracting the file list from the module map itself. This facilitates using modularize for regression testing also.

In the process of development, I also fixed a bug in the coverage check where it wasn't recursively collecting files in umbrella directories.

I updated the docs and tests accordingly.

This is staging for some coming enhancements, in particular adding diagnostics for tracking down cyclical module references. A couple of bug fixes are also coming.

This wasn't a particularly clean merge in terms of structure, but I'm under a bit of time pressure to beat the code freeze for a coming sdk release.

Once this is in, I'll delete the module-map-checker tool.

Diff Detail

Event Timeline

jtsoftware updated this revision to Diff 19515.Feb 6 2015, 2:47 PM
jtsoftware retitled this revision from to merge module-map-checker into modularize.
jtsoftware updated this object.
jtsoftware edited the test plan for this revision. (Show Details)
jtsoftware added reviewers: rsmith, silvas.
jtsoftware added a subscriber: Unknown Object (MLST).
silvas edited edge metadata.Feb 6 2015, 4:15 PM

This patch merges the module map coverage check from the existing module-map-checker tool into modularize.

Could you split this part out into a separate patch? Otherwise it is hard to review (i.e. delays review) since it's not clear what parts of the patch are what change. Moving module-map-checker into modularize should for the most part be a mechanical change where a reviewer can trivially see module-map-checker being deleted and moved into modularize.

It also allows a module map file to be used as the input to modularize, for doing the usual checks modularize does, but extracting the file list from the module map itself. This facilitates using modularize for regression testing also.

Please split this into a separate patch.

In the process of development, I also fixed a bug in the coverage check where it wasn't recursively collecting files in umbrella directories.

Please split this into a separate patch.

Sean,

The change for adding the recursion was a 4-line change:

@@ -374,9 +374,12 @@

std::string File(I->path());
I->status(Status);
sys::fs::file_type Type = Status.type();
  • // If the file is a directory, ignore the name.
  • if (Type == sys::fs::file_type::directory_file)

+ // If the file is a directory, ignore the name and recurse.
+ if (Type == sys::fs::file_type::directory_file) {
+ if (!collectUmbrellaHeaders(File))
+ return false;

continue;

+ }

// If the file does not have a common header extension, ignore it.
if (!isHeader(File))
  continue;

Because I basically copied a class from module-map-checker and hacked it a little, I can't really subdivide the patch any further.

-John

silvas added a comment.Feb 6 2015, 7:35 PM

A 4-line change in a sea of lines changed is impossible for a reviewer to see; please do it in a separate patch. Also, if this commit obsoletes the module-map-checker tool, please remove it in this same patch.

Also, teaching modularize to use a module map file as input to its (pre-module-map-checker-integration) duties is a logically separate change and should be done in a separate patch.

It's difficult for somebody to sign off on this patch with three (or more?) logically separate changes in it (especially when one of them is a very large mechanical change). There might be smaller changes that will have to be done before and after the large mechanical change (to prepare for the large mechanical change (e.g. to make it possible to actually have the large mechanical change be a single clear commit) or to better integrate things/clean things up once the large mechanical change has taken place).

jtsoftware abandoned this revision.Feb 10 2015, 5:22 AM

Got. Thanks.