wmi accepted this revision. wmi 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; + ---------------- tejohnson wrote: > 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 Thanks for making the comment more clear. 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
