sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/Headers.h:115
public:
+ using File = unsigned;
+
----------------
this concept needs documentation.
================
Comment at: clang-tools-extra/clangd/Headers.h:115
public:
+ using File = unsigned;
+
----------------
sammccall wrote:
> this concept needs documentation.
there's a naming issue here:
- "File" is an overloaded enough concept that you still end up referring to
these as "file indexes" below, which isn't clear.
- obviously this integer isn't either the file itself nor our record of it, so
the metaphor is a bit confusing
What do you think about just `ID`, which would be spelled from the outside as
`IncludeStructure::ID`?
Maybe `HeaderID` is better. I don't think it's a big problem that we have one
for the main file too...
================
Comment at: clang-tools-extra/clangd/Headers.h:117
+
+ llvm::Optional<File> getFile(StringRef Name) const {
+ auto It = NameToIndex.find(Name);
----------------
"Name" certainly needs to be clarified here.
Do you have any use cases in mind where you wouldn't have a FileEntry on hand?
That would make this typesafe (and in particular avoid passing absolute
filenames, or strings like "utility" or "<utility>)
================
Comment at: clang-tools-extra/clangd/Headers.h:124
+
+ llvm::ArrayRef<File> getIncludedFiles(File Index) const {
+ auto It = IncludeChildren.find(Index);
----------------
it's unclear from this patch whether you want to expose the whole graph or just
allow querying per-file - I doubt we need both.
================
Comment at: clang-tools-extra/clangd/Headers.h:124
+
+ llvm::ArrayRef<File> getIncludedFiles(File Index) const {
+ auto It = IncludeChildren.find(Index);
----------------
sammccall wrote:
> it's unclear from this patch whether you want to expose the whole graph or
> just allow querying per-file - I doubt we need both.
tests?
================
Comment at: clang-tools-extra/clangd/Headers.h:131
+
std::vector<Inclusion> MainFileIncludes;
----------------
this public member is now sandwiched between methods
================
Comment at: clang-tools-extra/clangd/Headers.h:141
// Usually it should be SM.getFileEntryForID(SM.getMainFileID())->getName().
- llvm::StringMap<unsigned> includeDepth(llvm::StringRef Root) const;
+ llvm::StringMap<unsigned> includeDepth(StringRef Name) const;
----------------
The public interface now uses the File abstraction, so this method sticks out.
(It has an awkward interface in the first place due to the need to use strings)
Suggest replacing it with `DenseMap<File, unsigned> includeDepth(File Root)`.
We'd need to add `StringRef RealPath(File)` for the caller, but that seems to
make sense anyway.
================
Comment at: clang-tools-extra/clangd/Headers.h:148
+ using AbstractIncludeGraph = llvm::DenseMap<File, SmallVector<File>>;
+
----------------
this isn't abstract :-)
also the public nature of it isn't used, in this patch at least.
================
Comment at: clang-tools-extra/clangd/Headers.h:151
private:
+ File getOrCreatefileIndex(llvm::StringRef Name);
+
----------------
File should be capitalized.
FWIW, for a private method I prefer the old name here
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D110386/new/
https://reviews.llvm.org/D110386
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
