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

Tue Mar 12 13:31:46 PDT 2019

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