https://github.com/HighCommander4 requested changes to this pull request.
Apologies for the slow response time. I finally had a chance to look at this in a bit more detail, though I still don't think I fully understand this patch. At this point, my main feedback is that it would be good to include some unit tests. `SymbolCollectorTests.cpp` is a good place for them; see e.g. [this test](https://searchfox.org/llvm/rev/78fa41524b6f6e2696ff21ec50e760311ac939a3/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp#413) for an example of testing on a header/source file pair as input, and [this test](https://searchfox.org/llvm/rev/78fa41524b6f6e2696ff21ec50e760311ac939a3/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp#1455) for an example of testing the value of the documentation field. What is the purpose of the change to `ASTContext.cpp`? This codepath has consumers other than clangd, so if we're changing the behaviour, we need to evaluate whether it's appropriate for other consumers (and maybe add libAST unit tests as well). https://github.com/llvm/llvm-project/pull/67802 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
