Page MenuHomePhabricator

modularize - header dependencies - version 2
ClosedPublic

Authored by jtsoftware on Aug 22 2013, 7:51 AM.

Details

Summary

Here's another crack at it using the options subsystems this time. (Thanks for the suggestions Sean and Manuel!)

The original summary:

In using modularize to check a large group of platform headers for modules-readiness, I found that a few headers had dependencies, such that they required other headers to be included first to avoid compile errors on missing definitions.

This patch adds support to modularize to allow specifying depended-on headers in the header file list input to modularize, i.e.

header.h: dependency1.h dependency2.h

Diff Detail

Event Timeline

I'm prolly not the right person to review whether this patch makes sense overall, but for the part that parses the options, that looks fine to me. Would probably be nice to put findInputFile (perhaps in the plural) somewhere more accessible...

Does this patch make sense from a design standpoint? Aren't these kinds of implicit dependencies the things that modularize is trying to get rid of in the first place? Why would we expend effort to enshrine them, when we can just detect them and emit an error?

I've also made a few comments about the code, but I think the above design issue is more important.

modularize/Modularize.cpp
281

Is the quoting here actually necessary? I don't think that there is a shell-escaping step after this.

287–302

Does this function actually use any of the class members? It seems like it should be a free function.

292–294

clang-format

doug.gregor accepted this revision.Sep 2 2013, 9:39 PM

The implementation looks fine.

Regarding Sean's question: applying modularize to a set of system headers can encounter a huge number of problems, not all of which are easy to fix. Adding a feature like this to modularize can help someone work around issues in the headers (which can also be worked around in the module map by listing headers in the appropriate order) to get better results from modularize with fewer widespread changes. For that reason, I think this is a useful feature to have. If we could fix all of the world's system headers, we would :(

jtsoftware closed this revision.May 5 2014, 6:11 PM

Committed in r189837 (and fixes r189983 r189984).

modularize/Modularize.cpp
281

It's needed because the path might have spaces (i.e. the default Sony sdk path does).

287–302

Got it.

292–294

Got it.