This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Follow-up on rGdea48079b90d
ClosedPublic

Authored by kbobyrev on Oct 1 2021, 5:23 AM.

Diff Detail

Event Timeline

kbobyrev created this revision.Oct 1 2021, 5:23 AM
kbobyrev requested review of this revision.Oct 1 2021, 5:23 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 1 2021, 5:23 AM
kbobyrev updated this revision to Diff 376488.Oct 1 2021, 5:26 AM

Get rid of SM in getOrCreateID & getID

sammccall accepted this revision.Oct 1 2021, 6:11 AM

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

This revision is now accepted and ready to land.Oct 1 2021, 6:11 AM
kbobyrev updated this revision to Diff 376811.Oct 3 2021, 11:35 PM
kbobyrev marked 9 inline comments as done.

Address review comments.

kbobyrev added inline comments.Oct 3 2021, 11:35 PM
llvm/include/llvm/Support/FileSystem/UniqueID.h
17

I think I do need utility because I'm using std::pair

This revision was automatically updated to reflect the committed changes.
sammccall added inline comments.Oct 5 2021, 12:58 AM
llvm/include/llvm/Support/FileSystem/UniqueID.h
17

I meant this file can include DenseMapInfo.h instead of DenseMap.h. Fixed in 4e91035387faf9e18134b1d46ce0fa803a6e9122

Ahh, sorry, I thought you meant the other one :( Apologies for confusion

Ahh, sorry, I thought you meant the other one :( Apologies for confusion

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.

ntfshard added a subscriber: ntfshard.EditedOct 21 2021, 7:12 AM

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.
getDevice and getFile returns uint64_t but pair expects unsigned ints.

kbobyrev marked an inline comment as done.Oct 21 2021, 7:27 AM
kbobyrev added inline comments.
llvm/include/llvm/Support/FileSystem/UniqueID.h
68

Still not sure, getHashValue function returns unsigned, but seems it should return hash_code or result of operator size_t()