tejohnson added inline comments.
================ Comment at: clang/lib/CodeGen/CGVTables.cpp:1301-1302 + // has no effect on the min visibility computed below by the recursive caller. + if (!Visited.insert(RD).second) + return llvm::GlobalObject::VCallVisibilityTranslationUnit; + ---------------- wmi wrote: > tejohnson wrote: > > wmi wrote: > > > If a CXXRecordDecl is visited twice, the visibility returned in the > > > second visit could be larger than necessary. Will it change the final > > > result? If it will, can we cache the visibility result got in the first > > > visit instead of returning the max value? > > The recursive callsites compute the std::min of the current TypeVis and the > > one returned by the recursive call. So returning the max guarantees that > > there is no effect on the current TypeVis. Let me know if the comment can > > be clarified (that's what I meant by "so that it has no effect on the min > > visibility computed below ...". Note that the initial non-recursive > > invocation always has an empty Visited set. > I see. That makes sense! I didn't understand the location meant by "computed > below by the recursive caller." Your explanation "initial non-recursive > invocation always has an empty Visited set" helps a lot. It means the > immediate result of GetVCallVisibilityLevel may change, but the result for > the initial invocation of the recursive call won't be changed. I've tried to clarify the comment accordingly Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91676/new/ https://reviews.llvm.org/D91676 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
