ckandeler wrote: > 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.
I've added a new test and verified that it fails without this patch. > What is the purpose of the change to `ASTContext.cpp`? There was some assumption about the order of redeclarations that did not match reality, like that the definition always comes first or something along those lines. I do not know whether the assumption there was wrong or whether some other code messed up by breaking it. > 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). Hence my original comment. Though it should be noted that no existing tests appear to have been broken. 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
