sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land.
Only really significant thing is I think the name "build graph" is confusing, and we should specifically mention includes. Rest is just nits to take or leave... ================ Comment at: clangd/Headers.h:52 +// Contains information about one file in the build grpah and its direct +// dependencies. +struct BuildGraphNode { ---------------- May be worth explicitly noting: `Does not own the strings it references (BuildGraphInclusions is self-contained)` ================ Comment at: clangd/Headers.h:53 +// dependencies. +struct BuildGraphNode { + bool IsTU; ---------------- I think we should be more specific and call this the "include graph". Build graph can suggest other build-system related things (bazel and ninja have a very formal concept of a build graph, other systems have less formalized but may still use the term) ================ Comment at: clangd/Headers.h:54 +struct BuildGraphNode { + bool IsTU; + llvm::StringRef FileURI; ---------------- this is worth a comment (e.g. `A "main file" as opposed to a header.`) ================ Comment at: clangd/Headers.h:55 + bool IsTU; + llvm::StringRef FileURI; + FileDigest Digest; ---------------- This is slightly ambiguous: URI of the file, or URI with the `file` scheme? I think you mean the former. I think `URI` is actually the best name for this field, even though it's taken by the class. As a member of a struct, there's not really any ambiguous cases. ================ Comment at: clangd/Headers.h:57 + FileDigest Digest; + std::vector<llvm::StringRef> FileInclusions; +}; ---------------- nit: I don't think "File" actually adds anything here, the whole struct is scoped to a file. I'd add a comment that this is only direct inclusions, if you choose not to mention it in the name. (The comment seems more useful here than it does on the struct) ================ Comment at: clangd/SourceCode.h:26 +using FileDigest = std::array<uint8_t, 20>; + ---------------- Probably worth a comment, especially since there are no functions here using it. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54817/new/ https://reviews.llvm.org/D54817 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits