jkorous added a comment. In D58749#1426270 <https://reviews.llvm.org/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, could you clarify? See detailed comments. Will do. >> This implementation is covered by lit tests using it through c-index-test in >> upcoming patch. > > This code looks very unit-testable. It also handles many corner cases (what > information is hashed and what is left out). c-index-tests are integration > tests that are not as good at that, and also they would be testing this code > quite indirectly. 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 writing tests against that data. What do you think? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58749/new/ https://reviews.llvm.org/D58749 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
