Differential D41102
Setup clang-doc frontend framework Authored by juliehockett on Dec 11 2017, 5:36 PM.
Details
Setting up the mapper part of the frontend framework for a clang-doc tool. It creates a series of relevant matchers for declarations, and uses the ToolExecutor to traverse the AST and extract the matching declarations and comments. The mapper serializes the extracted information to individual records for reducing and eventually doc generation. For a more detailed overview of the tool, see the design document on the mailing list: RFC: clang-doc proposal
Diff Detail Event TimelineThere are a very large number of changes, so older changes are hidden. Show Older Changes Comment Actions Next, i suggest to look into code self-debugging, see comments. I'm sorry that it seems like we are going over and over and over over the same code again,
Comment Actions The change to USR seems like quite an improvement already! That being said, I do think that it might be preferable to opt out of the use of strings for linking things together. What we did with our clang-doc is that we directly used pointers to refer to other types. So for example, our class for storing Record/CXX related information has something like: std::vector<Function*> mMethods; std::vector<Variable*> mVariables; std::vector<Enum*> mEnums; std::vector<Typedef*> mTypedefs; Only upon serialization we fetch some kind of USR that would uniquely identify the type. This is especially useful to us for the conversion to HTML and I think the same would go for this backend, as it seems this way you'll have to do string lookups to get to the actual types, which would be inefficient in multiple aspects. It can make the backend a little more of a one-on-one conversion, e.g. with one of our HTML template definitions (note: this is a Jinja2 template in Python): {%- for enum in inEntry.GetMemberEnums() -%}
<tr class="separator">
<td class="memSeparator" colspan="3"></td>
</tr>
<tr class="memitem:EAllocatorStrategy">
<td class="memItemLeft" align="right">{{- Modifiers.RenderAccessModifier(enum.GetAccessModifier()) -}}</td>
<td class="memItemMiddle" align="left">enum <a href="{{ enum.GetID() }}.html">{{- enum.GetName().GetName()|e -}}</a></td>
<td class="memItemRight" valign="bottom">{{- Descriptions.RenderDescription(enum.GetBriefDescription()) -}}</td>
</tr>
{%- endfor -%}Disadvantage is of course that you add complexity to certain parts of the deserialization (/serialization) for nested types and inheritance, by either having to do so in the correct order or having to defer the process of initializing these pointers. But see this as just as some thought sharing. I do think this would improve the interaction in the backend (assuming you use the same representation as currently in the frontend). Also, we didn't apply this to our Type representation (which we use to store the type of a member, parameter etc.), which stores the name of the type rather than a pointer to it (since it can also be a built-in), though it embeds pretty much every possible modifier on said type, like this: EntryName mName; bool mIsConst = false; EReferenceType mReferenceType = EReferenceType::None; std::vector<bool> mPointerConstnessMask; std::vector<std::string> mArraySizes; bool mIsAtomic = false; std::vector<Attribute> mAttributes; bool mIsExpansion = false; std::vector<TemplateArgument> mTemplateArguments; std::unique_ptr<FunctionTypeProperties> mFunctionTypeProperties = nullptr; EntryName mParentCXXEntry; The last member refers to the case where a pointer is a pointer to member, though some other fields may require some explaining too. Anyway, this is just to give some insight into how we structured our representation, where we largely omitted string representations where possible. Have you actually started work already on some backend? Developing backend and frontend in tandem can provide some additional insights as to how things should be structured, especially representation-wise!
Comment Actions Please run Clang-format and Clang-tidy modernize.
Comment Actions
Comment Actions I agree that the pointer approach would be much more efficient on the backend, but the issue here is that the mapper has no idea where the representation of anything other than the decl it's currently looking at will be, since it sees each decl and serializes it immediately. The reducer, on the other hand, will be able to see everything, and so such pointers could be added as a pass over the final reduced data structure.
I added you as a subscriber on the follow-up patches (the reducer, YAML/MD formats) -- would love to hear your thoughts! As of now, the MD output is very rough, but I'm hoping to keep moving forward on that in the next few days.
Comment Actions Could you please add a bit more tests? In particular, i'd like to see how blocks-in-blocks work. Is there some (internal to BitstreamWriter) logic that would 'assert()' if trying to output some recordid
Comment Actions Thank you for working on this!
Comment Actions
Comment Actions Yes -- it will fail an assertion:
Comment Actions
Comment Actions Um...no. Since you can have subblocks within blocks.
Comment Actions Running clang-format and fixing newlines
Comment Actions Thank you for working on this!
Comment Actions Thank you for working on this! Please consider adding even more tests (ideally, all this code should have 100% test coverage)
Comment Actions Adding tests, fixing comments, and removing an (as-of-yet) unused element of the CommentInfo struct.
Comment Actions Could some other people please review this differential, too? Some more nitpicking. For this differential as standalone, i'we mostly run out of things to nitpick. I'll try to look at the next differential, and at them combined.
Comment Actions Removing IsDefinition field.
Comment Actions My apologies for getting back on this so late! This seems like quite a decent approach! That being said, I don't see the pointer yet? I assume you mean that you will be adding this? Additionally, a slight disadvantage of doing this generic approach is that you need to do bookkeeping on what it is referencing, but I guess there's no helping that due to the architecture which makes you rely upon the USR? Personally I'd prefer having the explicit types if and where possible. So for now a RecordInfo has a vecotr of Reference's to its parents, but we know the parents can only be of certain kinds (more than just a RecordType, but you get the point); it won't be an enum, namespace or function. As I mentioned, we did this the other way around, which also has the slight advantage that I only had to create and save the USR once per info instance (as in, 10 references to a class only add the overhead of 10 pointers, rather than each having the USR as well), but our disadvantage was of course that we had delayed serialization (although we could arguably do both simultaneously). It seems each method has its merits :). Comment Actions If you take a look at the follow-on patch to this (D43341), you'll see that that is where the pointer is added in (since it is irrelevant to the mapper portion, as it cannot be filled out until the information has been reduced). The back references to children and whatnot are also added there.
The USRs are kept for serialization purposes -- given the modular nature of the design, the goal is to be able to write out the bitstream and have it be consumable with all necessary information. Since we can't write out pointers (and it would be useless if we did, since they would change as soon as the file was read in), we maintain the USRs to have a means of re-finding the referenced declaration. That said, I was looking at the Clangd symbol indexing code yesterday, and noticed that they're hashing the USRs (since they get a little lengthy, particularly when you have nested and/or overloaded functions). I'm going to take a look at that today to try to make the USRs more space-efficient here. Comment Actions Nice!
Comment Actions Oops! I'll have a look! What I was referring to was the storing of a USR per reference. Of course, serializing pointers wouldn't work, but what I mean is that what we used as a USR was stored in what was pointed to, not in the reference that tells what we are pointing to. To be a little more concise, a RecordInfo has pointers to the FuntionInfo for its member functions. Upon serialization, the RecordInfo queries the USR of those functions. A function being referenced multiple times remains to only have the USR stored. If I understand correctly, you currently save the USR for time an InfoType references another InfoType. Anyhow, don't pay too much attention to that comment, it's all meant as a minor thing. It sure is looking good so far! Comment Actions I'm sorry, brainfreeze, i meant 40 chars, not 20.
Comment Actions Updating bitcode writer for hashed USRs, and re-running clang-format. Also cleaning up a couple of unused fields. Comment Actions Hmm, i'm missing something about the way store sha1...
Comment Actions There's a few places where we can trim some of the boilerplate, which I think is important - it's hard to find the "real code" among all the plumbing in places.
Comment Actions Might have been better to not start landing until the all differentials are understood/accepted, but i understand that it is not really up to me to decide.
Comment Actions Since the commit was reverted, did you mean to either recommit it, or reopen this (with updated diff), so it does not get lost? Comment Actions Relanded in r327295.
Comment Actions So what part is failing, specifically? First one you should be able to handle by replacing the actual values with a regex Second one is interesting. if (DumpMapperResult) {
+ unsigned id = 0;
Exec->get()->getToolResults()->forEachResult([&](StringRef Key,
StringRef Value) {
SmallString<128> IRRootPath;
llvm::sys::path::native(OutDirectory, IRRootPath);
llvm::sys::path::append(IRRootPath, "bc");
std::error_code DirectoryStatus =
llvm::sys::fs::create_directories(IRRootPath);
if (DirectoryStatus != OK) {
llvm::errs() << "Unable to create documentation directories.\n";
return;
}
- llvm::sys::path::append(IRRootPath, Key + ".bc");
+ llvm::sys::path::append(IRRootPath, std::to_string(id) + ".bc");
std::error_code OutErrorInfo;
llvm::raw_fd_ostream OS(IRRootPath, OutErrorInfo, llvm::sys::fs::F_None);
if (OutErrorInfo != OK) {
llvm::errs() << "Error opening documentation file.\n";
return;
}
OS << Value;
OS.close();
+ id++;
});
}Comment Actions I was just thinking of disabling the one test that has an issue (class-in-function) on Windows -- the filename is only used in generating *some* USRs, so all of the other ones are fine. We ran into some issues with that though, since UNSUPPORTED: system-windows didn't seem to disable the test on the machine I have access to. Thoughts? Comment Actions
Perhaps that is only for msvc? Have you tried something more broad, like Comment Actions That wasn't working either, confusingly, at least on the local windows machine I have. Comment Actions Huh, something weird is going on there. Comment Actions After much digging, it looks like the lit config is never initialized in clang-tools-extra like it is in the other projects. REQUIRES et.al. work properly once that's in there (see D44708). Once that lands I'll reland this and *hopefully* that'll be that! | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
nit: you can also just write compile_flags.txt, which in this case would be empty