Details
- Reviewers
sammccall - Commits
- rGb06df223826e: [clangd] Follow-up on rGdea48079b90d
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Thanks! Just nits
(This review addresses comments on https://reviews.llvm.org/rGdea48079b90d40f2087435b778544dffb0ab1793)
clang-tools-extra/clangd/Headers.cpp | ||
---|---|---|
174 | if RealPathNames.front().empty... | |
clang-tools-extra/clangd/Headers.h | ||
123 | This is a strange public member, and would need to be documented. But it seems better yet to make collectIncludeStructureCallback a member (IncludeStructure::collect()?) so we can just encapsulate this. | |
125 | nit: this assertion feels a little out-of-place/unneccesary | |
153 | if we're going to expose the root headerID, we should do it in a named constant and reference that here. | |
166 | this member should be documented (I think you can just move the bit about HeaderID(0) from below to here. | |
166 | = nullptr | |
176 | nit: it's the *value* that's unstable, not the type. So just "the UniqueID" | |
177 | their -> the | |
llvm/include/llvm/Support/FileSystem/UniqueID.h | ||
17 | you only need DenseMapInfo here |
llvm/include/llvm/Support/FileSystem/UniqueID.h | ||
---|---|---|
17 | I think I do need utility because I'm using std::pair |
llvm/include/llvm/Support/FileSystem/UniqueID.h | ||
---|---|---|
17 | I meant this file can include DenseMapInfo.h instead of DenseMap.h. Fixed in 4e91035387faf9e18134b1d46ce0fa803a6e9122 |
All good. Instead of relying on us to spot this stuff, we should probably build some tooling to tell us which includes are needed and which are not.
It seems I faced with degradation in compile time.
edit: may be static_cast much better here
llvm/include/llvm/Support/FileSystem/UniqueID.h | ||
---|---|---|
68 | In this line a narrow conversion, and MSVC compiler can't build it with warnings as error key. |
llvm/include/llvm/Support/FileSystem/UniqueID.h | ||
---|---|---|
68 | Thank you for noticing! Fixed in https://github.com/llvm/llvm-project/commit/88303693ce97cf842f0714068c2cae44cd6515e1 |
Still not sure, getHashValue function returns unsigned, but seems it should return hash_code or result of operator size_t()
This is a strange public member, and would need to be documented.
But it seems better yet to make collectIncludeStructureCallback a member (IncludeStructure::collect()?) so we can just encapsulate this.