justincady wrote: Thank you for taking a look, @ArcsinX! I'll try to answer your questions to clarify this PR.
> We already have background-index.test, which checks that go-to-definition > request provides correct result with another file uri. Maybe > background-index.test is enough to be sure that paths inside shards are > correct? I was motivated to add an additional test to validate the actual on-disk contents. `background-index.test` validates the workflow, but does not explicitly validate what's being written out. > In you test we check that file names are correct, but not directories. Do we > need to cover the case when directories change? Forgive me, but I'm not following. No file names or directories are being changed. This test is validating that cracking open the respective .idx files reveals the expected data. > I am not sure that adding the test as a separate patch before the > functionality implementation makes sense. The community can object to this > new functionality. It's more typical to add the functionality and the test in > a single patch. I understand the concern. But since this test is _not_ related to the new functionality, it made sense to me to apply it independently. This change validates existing functionality. Even if my next PR never lands (which hopefully isn't the case!), this test adds value to clangd by confirming future changes do not unintentionally change the on-disk representation of background index files. I also wanted to help reviewers by keeping the upcoming PR for the new feature focused exclusively on that new feature. Perhaps it was my mistake to conflate the two by even mentioning future intent, and for that I apologize. If I've misunderstood or misrepresented your concerns, please feel free to correct me. Thanks! https://github.com/llvm/llvm-project/pull/179956 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
