ivanmurashko wrote: Thanks for the review, @NagyDonat. I am still working on addressing the "duplicates the traversal logic" comment and will return to the other comments right after the commit that addresses this major part of the refactoring. Overall, I plan to address all comments by the end of the week and have everything ready for review by Monday.
> > I reviewed the tests and added some minor suggestions in the implementation. Thank you very much. > > Among my earlier suggestions [the visibility of > VisitSymbol](https://github.com/llvm/llvm-project/pull/152751/files#r2301055454) > Ah, I am very sorry—it looks like my change was missed there. I will definitely add it. > and the [complex code duplication > question](https://github.com/llvm/llvm-project/pull/152751/files#r2301561854) > are still relevant. > > Moreover I thought about the approach that you currently emphasize "owning" > in every name and comment where you speak about smart pointers. As this is > not a distinguishing feature of these functions (you never interact with > non-owning smart pointers) and these function names tend to be very long, I > think it would be better to omit "owning" from these names. It is enough to > mention the exclusion of `weak_ptr` in a single comment (next to the function > that recognizes the names of the smart pointer classes). I also had doubts when choosing between the naming: with or without "Owning." I think it will be better to omit the word "Owning" to keep the names as simple as possible. https://github.com/llvm/llvm-project/pull/152751 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
