[PATCH] D58749: [index-while-building] IndexRecordHasher

jkorous added a comment. We did performance tests of alternative approach - just hashing the serialized bit code representation. There's a performance regression in the sense that while the current implementation costs approx. extra 2.2% in build time the alternative approach costs 3.8%. We ar

[PATCH] D58749: [index-while-building] IndexRecordHasher

jkorous added a comment. In D58749#1426769 , @kadircet > In D58749#1426778 , @gribozavr > I see what you mean now. That's a good idea. I'll add some unit tests. CHANGES SINCE LAST ACTION https://reviews.l

[PATCH] D58749: [index-while-building] IndexRecordHasher

jkorous marked 8 inline comments as done. jkorous added a comment. Addressed some comments, going to update the diff. Comment at: clang/lib/Index/IndexRecordHasher.cpp:291 + +hash_code IndexRecordHasher::hashImpl(const Decl *D) { + return DeclHashVisitor(*this).Visit(D); -

[PATCH] D58749: [index-while-building] IndexRecordHasher

jkorous updated this revision to Diff 190342. jkorous marked 2 inline comments as done. jkorous added a comment. Addressed some of Dmitri's comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58749/new/ https://reviews.llvm.org/D58749 Files: clang/lib/Index/CMakeLists.txt clang

[PATCH] D58749: [index-while-building] IndexRecordHasher

gribozavr added a comment. > I basically didn't really like the idea of testing against hard-coded hash > values in unittests as I consider it to be an implementation detail. Sorry, that's not what I was suggesting. There are better ways to test hashing. For example, write a piece of source c

[PATCH] D58749: [index-while-building] IndexRecordHasher

kadircet added a comment. > I basically didn't really like the idea of testing against hard-coded hash > values in unittests as I consider it to be an implementation detail. I was > thinking about integration tests that would work around this by both writing > index and reading index and writin

[PATCH] D58749: [index-while-building] IndexRecordHasher

jkorous added a comment. In D58749#1426270 , @gribozavr wrote: > I left some comments, but it is difficult for me to review without > understanding what the requirements for this hasher are, why some information > is hashed in, and some is left out, coul

[PATCH] D58749: [index-while-building] IndexRecordHasher

jkorous updated this revision to Diff 190317. jkorous added a comment. Herald added a subscriber: jfb. Based on Kadir comment I refactored the code. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58749/new/ https://reviews.llvm.org/D58749 Files: clang/lib/Index

[PATCH] D58749: [index-while-building] IndexRecordHasher

gribozavr added a comment. I left some comments, but it is difficult for me to review without understanding what the requirements for this hasher are, why some information is hashed in, and some is left out, could you clarify? See detailed comments. > This implementation is covered by lit test

[PATCH] D58749: [index-while-building] IndexRecordHasher

kadircet added inline comments. Comment at: clang/lib/Index/IndexRecordHasher.h:41 + /// Returns hash for all declaration occurences in \c Record. + llvm::hash_code hashRecord(const FileIndexRecord &Record); + llvm::hash_code hash(const Decl *D); Why expose ha

[PATCH] D58749: [index-while-building] IndexRecordHasher

jkorous added reviewers: kadircet, gribozavr. jkorous added a comment. Adding clangd folks in case they want to take a look. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58749/new/ https://reviews.llvm.org/D58749 __

[PATCH] D58749: [index-while-building] IndexRecordHasher

jkorous created this revision. jkorous added reviewers: nathawes, akyrtzi, arphaman, dexonsmith, ioeric, malaperle. Herald added subscribers: cfe-commits, jdoerfert, mgorny. Herald added a project: clang. Another piece of index-while-building functionality. RFC: http://lists.llvm.org/pipermail/cf