[clang] [clang][doc] Add documentation for the ASTs used to represent C++ templates (PR #66436)
shafik wrote: I would hate to lose the non-diagram part of this documentation. Do the criticism mainly apply to the diagrams and the text documentation is ok for most? https://github.com/llvm/llvm-project/pull/66436 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][doc] Add documentation for the ASTs used to represent C++ templates (PR #66436)
https://github.com/smcpeak closed https://github.com/llvm/llvm-project/pull/66436 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][doc] Add documentation for the ASTs used to represent C++ templates (PR #66436)
smcpeak wrote: As the feedback has been mostly negative, I'm closing this PR. https://github.com/llvm/llvm-project/pull/66436 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][doc] Add documentation for the ASTs used to represent C++ templates (PR #66436)
smcpeak wrote: And more generally, is there a preferred overall approach to documenting AST structures? My basic idea was to follow the example set by the [The AST Library](https://clang.llvm.org/docs/InternalsManual.html#the-ast-library) in the Internals Manual, but expanded to cover templates. I'm open to doing something completely different, for example primarily or exclusively adding Doxygen comments. The goal is simply to make it easier for someone new to learn the ASTs used to represent templates. https://github.com/llvm/llvm-project/pull/66436 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][doc] Add documentation for the ASTs used to represent C++ templates (PR #66436)
smcpeak wrote: Is there a different tool and/or style of diagram that would be preferable? https://github.com/llvm/llvm-project/pull/66436 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][doc] Add documentation for the ASTs used to represent C++ templates (PR #66436)
erichkeane wrote: Even after seeing your explaination, I don't find the images particularly useful or helpful. They are convoluted and confusing, and I don't think they add value here. As far as being checked in: First, Git does really poorly with binary files. Second, if we were doing something like this, I'd request/require/demand/etc that it be published as a part of the documentation build, similar to how we do our Sphinx build documentation, not in-tree. It wouldn't require developers changing documentation to have the tool/dependency same as we don't really require that for Sphinx today, only a build-bot would be bothered. The synchronization of them is a massive concern that your explanation doesn't really cover: you say that checking them in is preferable since otherwise we have a dependency on 'ded', yet checking them in means the devs have to do it/have the tool. From the looks, 'ded' is a textual representation (though not a particularly maintainable looking one), that I'd suspect folks would want to make changes to with a text tool. In the end, the value proposition here doesn't check out. I can see SOME value to having a graphical AST representation, but I don't think this tool/interface is the way to do it, at least as far as I can see. https://github.com/llvm/llvm-project/pull/66436 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][doc] Add documentation for the ASTs used to represent C++ templates (PR #66436)
smcpeak wrote: # On the utility of the diagrams I'm surprised this is a point of contention. Could someone elaborate on why the diagrams are troublesome? To me, saying that the diagrams are not useful is sort of like saying the `-ast-dump` output is not useful. Isn't it better to be able to see all of the relevant AST objects in play at once? Perhaps the problem is simply that the diagram notation is not adequately explained. I see that I never did explain it; that was an oversight. I therefore propose to add the following text right after the first diagram, in the section "Diagram: Function template: Definition". Would this addition resolve objections to the diagrams? Should I add even more detail about the notation, for example, discussing each of the objects, pointers, and fields that is shown individually? ## Proposed additional text below ft-defn.ded In this diagram, each box (except "Source Code") represents a single object in the AST that was built by the Clang parser and semantic analyzer, each arrow represents a single pointer from one AST object to another, and each item inside a box represents a single data field of the containing object. For example, the "TU" box represents the `TranslationUnitDecl` object. That object has a pointer called `DeclContext::LastDecl` that points at the `FunctionTemplateDecl 14` object. The `FunctionTemplateDecl 14` object has a field called `Name` with value `"identity"` when rendered as a string. Object names include their type, such as `FunctionTemplateDecl`, and a unique numeric identifier (such as `14`) to distinguish them from other objects with the same type and facilitate naming them in the prose. The diagram can be understood as a complement to the `-ast-dump` output, which instead has one line per object. The diagram does not show all of the AST objects because that would be too much clutter, and the point is to focus on a few key interactions. But for the nodes that are shown, the diagram includes details that `-ast-dump` omits, especially the pointer relations that are not part of the tree backbone. # On generating the diagrams during the build I think it is preferable to check in the PNG files than to regenerate them on every build. Regenerating them would mean all developers who change documentation would need the `ded` tool, which is written in Java. If it's checked in to Clang, that means adding a JDK dependency (as Clang currently has no Java code). If not, the editor itself is then an extra (documentation) build-time dependency for people to obtain and install. Regenerating the PNGs avoids a potential issue where the DED and PNG are out of sync, but this is unlikely to happen in practice because the `ded` tool always saves both at the same time. Regarding size, the PNG files are usually not larger than the DED files, so removing them will not save a lot of space in the repo. Also, if space is a concern, both the DED and PNG files can be "trimmed" somewhat, removing elements of the underlying AST graph that are not shown in the diagram, thereby reducing file size (by about half for the larger DEDs, and maybe 10% for the PNGs). I didn't do that in this initial submission to make it easier for others to add information to a diagram if they thought something was missing. Furthermore, it is possible to remove the DED files entirely, since `ded` can directly edit the PNG files without loss of information (the contents of the DED file are embedded as a compressed comment in the PNG). The reason to keep the DED files is it's possible to see what has changed when reviewing changes in `git` and, in some cases, merge conflicting changes automatically (and even when they cannot be merged automatically, it's helpful to see the diffs in order to perform a manual merge). https://github.com/llvm/llvm-project/pull/66436 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][doc] Add documentation for the ASTs used to represent C++ templates (PR #66436)
erichkeane wrote: > TBH, not sure if the UML diagrams would really help anyone understand what's > going on. I tend to agree with this, the documentation via these diagrams is confusing me as well, it doesn't really seem to add anything (and in fact, seem to be 'subtracting' in my case). That said, if we DO decide to do this, we shouldn't commit the PNG, we need to add building of these from the script(.ded file) to cmake. https://github.com/llvm/llvm-project/pull/66436 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][doc] Add documentation for the ASTs used to represent C++ templates (PR #66436)
smcpeak wrote: Hi Tom! :) Indeed, any documentation brings with it an ongoing maintenance burden. There are two main things I see that need synchronization: the text descriptions of the fields, and the diagrams. For the fields, one idea I had is to write a script that runs as part of the tests and would scrape the declarations (and maybe the Doxygen comments too) from the C++ source and then compare them to previously scraped fragments embedded as comments in `ASTsForTemplates.rst`. The idea is if a declaration that is documented changes, the script would alert the one making the change that the corresponding documentation might need to be adjusted. After adjusting as needed, the scraped fragment in the document would be updated (by the same person), thereby returning to the "in sync" state. For the diagrams, they are generated in a semi-automatic way: a tool (`print-clang-ast`) generates a graph of AST nodes, then the diagram selectively imports nodes, attributes, and pointers from that graph. If the tool were in the Clang repo, it would be straightforward to, for each diagram, have it regenerate the graph and compare to the graph stored in the diagram file. This comparison can be done without using the `ded` program because the entire diagram, including the embedded graph, is in a JSON format. If the graphs are found to differ, it means the diagram needs to be updated; that *would* required `ded`, which has features to reimport an updated graph, perform localized repairs, and check consistency between the diagram and graph. I'd be willing to do both of the above if people think either or both is a good approach. As for the utility of the diagrams, I think they are the most important part of the document since they allow the reader to see how all of the elements work together, solidifying the material in the text that precedes them. Studying the diagrams was the primary way I learned this material in the first place. https://github.com/llvm/llvm-project/pull/66436 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][doc] Add documentation for the ASTs used to represent C++ templates (PR #66436)
tahonermann wrote: Hi, Scott! It's nice to see you pop up here! :) I'm a little concerned about maintenance of this documentation going forward. As you know, the Clang AST is not intended to be stable and, even though I don't expect dramatic changes to how templates are represented, it will take some effort to ensure the code and the documentation stay in sync. Do you have any thoughts regarding how we can implement an automated process to ensure the documentation is updated? For anyone not familiar, [ded](https://github.com/smcpeak/ded) is a diagram editor that Scott developed. I've used it in the past and found it to be lightweight and easy to use. https://github.com/llvm/llvm-project/pull/66436 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][doc] Add documentation for the ASTs used to represent C++ templates (PR #66436)
tbaederr wrote: TBH, not sure if the UML diagrams would really help anyone understand what's going on. https://github.com/llvm/llvm-project/pull/66436 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][doc] Add documentation for the ASTs used to represent C++ templates (PR #66436)
https://github.com/shafik review_requested https://github.com/llvm/llvm-project/pull/66436 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][doc] Add documentation for the ASTs used to represent C++ templates (PR #66436)
https://github.com/shafik review_requested https://github.com/llvm/llvm-project/pull/66436 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][doc] Add documentation for the ASTs used to represent C++ templates (PR #66436)
https://github.com/shafik review_requested https://github.com/llvm/llvm-project/pull/66436 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][doc] Add documentation for the ASTs used to represent C++ templates (PR #66436)
https://github.com/llvmbot labeled https://github.com/llvm/llvm-project/pull/66436 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
