This is based on the discussions from the LLVMDev thread:
http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-August/075886.html
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
I think the API used by the pass is really good here. Maybe some *minor* tweaks, but generally, this seems fine. Totally fine to commit.
Most of my comments below are on the implementation of things. The big thing I think should get addressed before this goes in are docs for the option API so folks know how to use it, and sinking the cl::Option stuff into Options.cpp (if that proves possible).
Thanks, and sorry for the delays here.
include/llvm/Support/CommandLine.h | ||
---|---|---|
35–48 ↗ | (On Diff #13814) | Most of this is unused now? I would reverse the dependency relationship here. I would have Option.h provide the key type as a member type of the registry, and include it here. Then I would sink tall of the cl::Option building stuff into the implementation file. If this proves hard, feel free to leave the OptionKey business here, but nuke the rest. |
182 ↗ | (On Diff #13814) | If you end up needing this, just remove the 'llvm::' and you won't need to forward declare the class. |
include/llvm/Support/Options.h | ||
1 ↗ | (On Diff #13814) | Need normal file template... |
7 ↗ | (On Diff #13814) | Doesn't seem like a useful comment... |
14–15 ↗ | (On Diff #13814) | What are the ownership semantics here? i feel like these really need better comments. |
17 ↗ | (On Diff #13814) | Not a useful comment. |
29 ↗ | (On Diff #13814) | Usually "I" or "It" but not "IT". |
lib/Support/Options.cpp | ||
17–21 ↗ | (On Diff #13814) | Oof, this is the only thing requiring the cl::Option to be aware of the key? Do we really need this API? Considering that the registration is expected to happen during "initialization" my inclination is to make it monotonic and remove the ability to unregister options. Does that make sense to you? |
lib/Transforms/Scalar/Scalarizer.cpp | ||
154–156 ↗ | (On Diff #13814) | Should re-wrap comment. |
Thanks for the feedback. Sorry it has taken me a while to reply. The suggestions all look good. I'm working on revising the patches, and will upload an update shortly.
More comments below.
include/llvm/Support/CommandLine.h | ||
---|---|---|
35–48 ↗ | (On Diff #13814) | The opt_builder was definitely just something I missed cleaning up my earlier patches, and if I rip out the removeOption code as you suggest I can move the LLVMOptionKey to Options.h and get rid of the rest of this. Ultimately I'd like to invert the relationship as you mention and have all of cl::Option be in Options.h, but I'd like to leave that for later when we can re-design how the innards of all the cl::opt stuff works. Does that sound reasonable to you? |
lib/Support/Options.cpp | ||
17–21 ↗ | (On Diff #13814) | I think it is reasonable to remove the ability to remove options. |
These patches include updates based on Chandler's feedback:
- Now there are no changes to CommandLine.h
- Options.h and Options.cpp have minor code cleanup and style adjustments
- OptionRegistry no longer supports removing options. This support wasn't really used anyways.
- Formatting fixup in Scalarizer.cpp
I noticed a few comments that were incorrect in the new file headers. These patches update those headers.
Again, the API in the pass seems pretty good. More comments on the implementation.
include/llvm/IR/LLVMContext.h | ||
---|---|---|
167–170 ↗ | (On Diff #14345) | This needs some documentation. =] |
include/llvm/PassSupport.h | ||
87 ↗ | (On Diff #14345) | We should use the new naming conventions here so we aren't establishing new non-conventional API requirements. |
include/llvm/Support/Options.h | ||
21–22 ↗ | (On Diff #14345) | This comment doesn't really make sense to me. Not sure what its trying to say... Maybe just: 'Options are keyed off the unique address of a static character, potentially one synthesized based on a particular set of template arguments'? |
23 ↗ | (On Diff #14345) | Not sure giving this type a name really helps. I would just use a 'void *' as the key type internally, and the class template as the external interface? If you want to make the 'void *' raw key type available for clients, I would typedef 'OptionKey' to 'void *', and name the template for synthesizing a static variable whose address is a suitable 'void *' key for a set of template arguments something like 'OptionKeyFromTemplateArgs'. |
31 ↗ | (On Diff #14345) | Why initialize it at all? |
33–35 ↗ | (On Diff #14345) | Please use modern doxygen syntax here and elsewhere. Also it would be good to give the class a reasonable introductory comment. |
38 ↗ | (On Diff #14345) | Should document that this owns the cl::Option objects pointed to but doesn't use std::unique_ptr or delete them but instead intentionally leaks them. |
45–46 ↗ | (On Diff #14345) | Shouldn't these be private? Only a single global instance of this registry can exist in order to make the ownership semantics of the cl::Option objects reasonable? |
One comment below. I will update the patches to reflect the other feedback.
include/llvm/Support/Options.h | ||
---|---|---|
45–46 ↗ | (On Diff #14345) | These can't be private unless I declare ManagedStatic as a friend, because they are invoked from ManagedStatic, which I think somewhat defeats the point. |
A few more comments for things I came across while updating the patches. Updated patches coming shortly.
include/llvm/Support/Options.h | ||
---|---|---|
31 ↗ | (On Diff #14345) | If you don't initialize this you get a warning "has internal linkage but is not defined [-Wundefined-internal]" |
38 ↗ | (On Diff #14345) | But it does delete them... See the destructor in Options.cpp. |
I've removed the typedef as per chandlerc's comments, and I've updated the formatting and added comments throughout.
Mostly just getting the comments cleared up with good documentation for this new API.
include/llvm/IR/LLVMContext.h | ||
---|---|---|
167–168 ↗ | (On Diff #14750) | Again, follow the modern guidelines for doxygen. |
include/llvm/Support/Options.h | ||
10–12 ↗ | (On Diff #14750) | I would turn this into a doxygen comment using the '\file' directive. See iterator_range.h for an example. I would also specifically add an example of how to use this API to add an option to the pass. |
21–29 ↗ | (On Diff #14750) | I would put all of this into a 'detail' namespace. Users of the API shouldn't really be aware of it. |
31 ↗ | (On Diff #14750) | A brief summary here would really help. Also, things like "This class" don't really contribute much. For example: /// \brief Singleton class used to register debugging options. /// /// ... |
39–40 ↗ | (On Diff #14750) | Modern doxygen. |
47 ↗ | (On Diff #14750) | Modern doxygen. I'm going to stop making this comment, but *please* actually check all your API documentation. |
This update contains changes based on chandlerc's feedback.
Changes in these patches:
- Moved the OptionKey into a detail namespace
- Updated comments based on chandlerc's feedback
Since the API seems stabilized and the feedback at this point is mostly documentation can we transition this to post-commit review for further feedback?
Sorry for the delay, but I do think it is really important to get basic API documentation right.
Anyways, this looks much better as an initial iteration on the API. Thanks, and please commit.
include/llvm/Support/Options.h | ||
---|---|---|
9 ↗ | (On Diff #14963) | its '\file' |
Thanks! I agree getting the docs right is important, and I'm really happy with where this API has ended up.