zixuw added a comment. Herald added a subscriber: StephenFan. Hi Sam! Thanks for your interest and taking a look!
In D121873#3391059 <https://reviews.llvm.org/D121873#3391059>, @sammccall wrote: > Hey, apologies for missing the initial patch in D119479 > <https://reviews.llvm.org/D119479>. > > This sounds really interesting! > The lack of docs make it very difficult to understand what this library is > for and how to use it if you don't already know. > It's not clear what the SymbolGraph library is (I think there's a doc missing > from the directory). The patches are tagged with [extract-api] so it must be > related, but if it's something distinct it should be documented and reusable, > and if it's the same why is it not called ExtractAPI :-) My apologies for the confusion caused! Yes I do need to improve the docs for existing changes and put more efforts into the readability of the code in future patches. And I definitely agree that SymbolGraph is not really an appropriate name for the library. Initially the code was directly put in clangFrontend but I had to move them out to resolve a circular dependency problem between frontend and clangIndex. And it seems that I'm just bad at naming things 🙂. I've put up another patch D122160 <https://reviews.llvm.org/D122160> to refactor all the ExtractAPI work and also add missing doc comments to make things clearer. > The main entry point seems to be API.h, which has only a trivial file comment > and no comments on what the meaning of the structs and fields are. Especially > if this is part of clang proper, it's going to be read and maintained and > debugged by a lot of people who have never heard of SymbolGraph and don't > know swift. > > It's also not clear from looking at this why this is part of clang itself and > not a libTooling-style external tool - it seems similar in scope and > applicability to a lot of the things in clang-tools-extra. Implementing this with libTooling in clang-tools-extra is definitely something we've considered and evaluated during designing and before the proposal. And we decided that including this in clang core had great benefits: - Implementing ExtractAPI in clang itself allows the functionalities to be easily accessible by more downstream tools to use either the API information collected or the serialized output via clang or libclang, for example IDE integrations. - We could share parts of the code with Swift down the line. - The proposed clang-extract-api tool only need to parse a set of given header files for flexibility and efficiency. Implementing ExtractAPI as a clang driver allows a more flexible control of invoking the tool with various of clang options without the need of a compilation database, and be decoupled from compilations and builds. > I know a lot of these things don't feel like they need much explanation when > you're and your reviewer are familiar with it, so apologies that this is > tedious, but the cost of undocumented code like this in shared projects is > high. Not at all! There are all valid and great feedbacks and questions. Thank you very much for pointing there out Sam! I've definitely neglected documenting and presenting the code for better readability and community collaboration. It's always welcome and exciting to have these feedbacks to move the tool forward together. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121873/new/ https://reviews.llvm.org/D121873 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
