[PATCH] D54817: [clangd] Put direct headers into srcs section.

[Sam McCall via Phabricator via cfe-commits]

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