wmi accepted this revision. wmi added a comment. This revision is now accepted and ready to land.
LGTM. ================ 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: > > 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. 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
