[PATCH] D41102: Setup clang-doc frontend framework

Athos via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Feb 6 03:34:06 PST 2018


Athosvk added inline comments.


================
Comment at: tools/clang-doc/ClangDocReporter.h:39
 // Info for named types (parameters, members).
 struct NamedType {
   std::string Type;
----------------
Storing the type information seems more suitable than storing just the name and type as a string. In my view, the frontend creates a format suitable for (almost) any backend to use without further parsing. This would for example require me to parse part of the name to get the namespace.


================
Comment at: tools/clang-doc/ClangDocReporter.h:75
 struct Info {
   bool isDefined = false;
   std::string FullyQualifiedName;
----------------
NItpick but should probably be 'IsDefined'


================
Comment at: tools/clang-doc/ClangDocReporter.h:87
   std::string MangledName;
   std::string DefinitionFile;
   std::string ReturnType;
----------------
Seems common to almost all Info structs, so you can probably move it to the/some base.

Namespaces do seem unrelated, so maybe you can make another struct inbetween? E.g. something like struct SymbolInfo : Info which contains a field for DefinitionFile and Locations (since that may not be used for namespaces either).

Additionally, what will you do when you merge this output information from multiple compilation untis back together? Only one should have the DefinitionFile set as the other compilation units won't see the definition. What happens if a function stays undefined? Can you generate documentation for it?


================
Comment at: tools/clang-doc/ClangDocReporter.h:88
   std::string DefinitionFile;
   std::string ReturnType;
   llvm::SmallVector<NamedType, 4> Params;
----------------
This should probably be a NamedType like the parameters


================
Comment at: tools/clang-doc/ClangDocReporter.h:89
   std::string ReturnType;
   llvm::SmallVector<NamedType, 4> Params;
   AccessSpecifier Access;
----------------
Perhaps you could already attach the parameter comments to this? So something like a 

```
struct ParamInfo
{
    NamedType Type;
    std::string/CommentInfo Description/CommentInfo;
}
```
Or are you planning to keep this to a later stage? At this point it seems like the backend will have to parse a CommentInfo struct to attach comments to parameters etc. manually


================
Comment at: tools/clang-doc/ClangDocReporter.h:93
 
 struct NamespaceInfo : public Info {
+  llvm::StringMap<std::unique_ptr<FunctionInfo>> Functions;
----------------
Currently info stores the locations of occurrences, but this seems like a hard thing to do when it comes namespaces. Is it useful to have this particular information?


https://reviews.llvm.org/D41102





More information about the cfe-commits mailing list