Page MenuHomePhabricator

Modularize assistant mode
ClosedPublic

Authored by jtsoftware on Oct 10 2013, 11:57 AM.

Details

Summary

The patch features a new mode for modularize in which it generates a module map file as a starting point for a real module.map file, based on the header list input to modularize.

Please see the file comments for details.

Diff Detail

Event Timeline

Do you think that this may benefit in the future from being interactive?

modularize/ModuleAssistant.cpp
33–39

http://llvm.org/docs/CodingStandards.html#include-style (the most "local" headers go first).

40–41

I don't think I've ever a namespace explicitly opened in a .cpp file in the LLVM codebase. Usually there is just a "using namespace Modularize", which works when you then define Module::foo.

All classes that are actually declared in the .cpp file (e.g. class Module and class ModuleAssistantImpl below) are only visible in this .cpp file and should be enclosed in an anonymous namespace http://llvm.org/docs/CodingStandards.html#anonymous-namespaces.

96

Ew. Can you use raw_ostream::indent to achieve this more cleanly OS.indent(...) << ...? Or maybe a member function getIndent()?

99–100
143

LLVM style uses 0 for the null pointer. E.g. http://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code. Of course, once we have C++11, nullptr is the obvious thing.

153–159

1-3 here are really helpful comments. 4 is not as helpful; can you improve 4?

205–208

can fold this to be just if (!addModuleDescription(*I, ...

229–231

Will the user understand what "dependents" means in this context? I sure don't.

266–267

for (int Index = ...

329–331

Ew. Can you use OwningPtr to capture these lifetime semantics? http://llvm.org/docs/doxygen/html/classllvm_1_1OwningPtr.html

modularize/ModuleAssistant.h
10–11 ↗(On Diff #4819)

This is really unhelpful. At least give the reader a little guidance.

25–36 ↗(On Diff #4819)

Do you expect to have multiple "module assistant" implementations? Would just a single free function

bool
createModuleMap(llvm::StringRef ModuleMapPath,
                llvm::SmallVectorImpl<std::string> &HeaderFileNames,
                DependencyMap &Dependencies, llvm::StringRef HeaderPrefix,
                llvm::StringRef RootModuleName)

be sufficient for now?

29–34 ↗(On Diff #4819)

Please add a doxygen comment describing each parameter.

32 ↗(On Diff #4819)

ArrayRef is probably better unless you plan on appending/modifying the SmallVector.

33 ↗(On Diff #4819)

Where is this DependencyMap type coming from? It doesn't seem to be in any of the #includes?

Thanks! I'll post an updated revision.

modularize/ModuleAssistant.cpp
40–41

Wow, I've been writing C++ for 24 years, and I never knew this about namespaces. Intuitively it seems wrong that you can implement class members outside of the namespace, but I bow to convention.

I put the internals in an anonymous namespace. It seems the conventions say that anonymous namespaces are only for small private chunks, but perhaps this is small enough.

96

Got it.

99–100

Got it.

143

Got it.

153–159

Got it.

205–208

Got it.

229–231

Got it.

266–267

Got it.

329–331

Got it.

modularize/ModuleAssistant.h
10–11 ↗(On Diff #4819)

Got it.

25–36 ↗(On Diff #4819)

Got it. Made a free function, though it relays to an implmentation.

32 ↗(On Diff #4819)

Got it.

33 ↗(On Diff #4819)

I added a Modularize.h file to hold definitions common to more than one file, like this one.

jtsoftware updated this revision to Unknown Object (????).Oct 11 2013, 7:21 AM

I addressed all the inline comments.

silvas added inline comments.Oct 11 2013, 2:15 PM
modularize/ModuleAssistant.cpp
169–173

Looking at it again, this seems like it should just be a free function with the tool_output_file stack-allocated in it, and passing just the tool_output_file's raw_ostream to the ModuleAssistantImpl constructor. Something like:

llvm::SmallString<256> FilePath;
getModuleMapOutputPath(ModuleMapPath, HeaderPrefix, FilePath)
std::string Error;
tool_output_file Out(FilePath.c_str(), Error);
if (!Error.empty()) { ...; return; }
...

Is ModuleAssistantImpl necessary? It doesn't seem to be holding any significant state.

Side note: Notice how there is a "long-distance resource-ownership dependency" between setUpModuleMapOutput and finalizeModuleMapOutput; usually this indicates that the resource that causes those two routines to be coupled should become a stack-allocated variable, or it should become a by-value member of a class whose lifetime matches the pairing of the two coupled functions.

251

It doesn't look like this has its lifetime managed (same with RootModule above). Maybe you could just allocate these into a BumpPtrAllocator?

261–266

Please mention how this list was derived, so that it's clear how to determine if it needs updating (and how to update it).

268–278

This doesn't depend on any of the class's state, so just make it a static helper function.

Also, since you are intending to modify MightBeReservedName, it probably makes more sense to take a SmallVectorImpl&. That would also simplify the interface. The calling sequence would just be ensureNoCollisionWithReservedName(S);

283–297

this should be a static helper function.

333–335

FYI, the whole point of using an OwningPtr was so that you wouldn't need to manually free it ;)

jtsoftware updated this revision to Unknown Object (????).Oct 14 2013, 7:16 AM

This update addresses the comments on the previous revision, except for revising the ensureNoCollisionWithReservedName argument, which I couldn't understand. Also, I eliminated the ModuleAssistant.h file, merging it with Modularize.h, since it only declared one function.

LGTM with a couple tiny changes. Doug, could you take a look at this?

Btw, does this need user-facing documentation yet? (that can be a separate commit if so)

modularize/ModuleAssistant.cpp
267–268

I don't think you need to explicitly say this.

273

The rationale in http://llvm.org/docs/CodingStandards.html#anonymous-namespaces is basically to improve locality of reference. This anonymous namespace should surround just the declaration of class Module, and the free functions that currently are in the anonymous namespace (but will not be after that change) should be marked static.

jtsoftware updated this revision to Unknown Object (????).Oct 14 2013, 11:13 AM

Thank you!

Revised anonymous namespace as suggested, and removed unnecessary comment.

The user docs will follow.

doug.gregor accepted this revision.Oct 21 2013, 8:59 PM

This looks really helpful!

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

Committed in r191211 (docs in r192703).