[PATCH] D155659: [WPD][LLD] Add option to validate RTTI is enabled on all native types and prevent devirtualization on types with native RTTI

[Di Mo via Phabricator via llvm-commits]

modimo marked 27 inline comments as done.
modimo added a comment.

In D155659#4647141 <https://reviews.llvm.org/D155659#4647141>, @MaskRay wrote:

> Looks great with some nits! Checking `_ZTS` in `TypeIDVisibleToRegularObj` (switch to `typeIDVisibleToRegularObj`) looks good to me. Sorry for the delay.
> There are a number of resolved comments you may want to mark as done.

Appreciate the thorough review! Good callout on the resolved comments, keeping those up to date keeps the context clearer for everyone involved--will keep them updated in the future.



================
Comment at: llvm/lib/LTO/LTO.cpp:1298
+  auto IsVisibleToRegularObj = [&](StringRef name) {
+    return GlobalResolutions.count(name)
+               ? GlobalResolutions[name].VisibleOutsideSummary
----------------
MaskRay wrote:
> redundant hash table lookup here.
> 
> Better to use `find(name)` with slightly more code
Good call, other places here also use the find pattern.


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:787
 
+bool TypeIDVisibleToRegularObj(
+    StringRef TypeID, function_ref<bool(StringRef)> IsVisibleToRegularObj) {
----------------
MaskRay wrote:
> Perhaps we should fix these functions to follow https://llvm.org/docs/CodingStandards.html#use-namespace-qualifiers-to-implement-previously-declared-functions . Pushed b4d4146db3b9a29773259c8b8a6cb7c98da90e73 and you'll need a rebase.
Sounds good, I'll follow up with the correct style on the rebase


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155659/new/

https://reviews.llvm.org/D155659