[PATCH] D149860: [TextAPI] Introduce SymbolSet

[Cyndy Ishida via Phabricator via llvm-commits]

cishida added inline comments.


================
Comment at: llvm/include/llvm/TextAPI/InterfaceFile.h:346
+  /// \param Name The name of the symbol.
+  std::optional<const Symbol *> contains(SymbolKind Kind,
+                                         StringRef Name) const {
----------------
zixuw wrote:
> nit: why do we need an `std::optional` instead of using the nullptr to indicate non-existence? And if the method returns the query element maybe we should give it another name than `contains`?
`nullptr` and `std::nullopt` evaluate differently in e.g. `if (auto x = file->contains(sym))`. I don't feel that strongly about supporting but it is used that way downstream. 
I'll update the name though.


================
Comment at: llvm/include/llvm/TextAPI/Symbol.h:73-75
+      lower_bound(Container, Targ, [](const Target &LHS, const Target &RHS) {
+        return LHS < RHS;
+      });
----------------
zixuw wrote:
> nit: I think `lower_bound` already uses `operator<` for comparison. Is this needed? Or maybe use `std::less{}`?
This is just moved from `InterfaceFile.cpp`, I realized I forgot to delete that in this patch (will fix that) but I'd rather not update this code unless theres a strong reason to since its used in a lot of tapi data structure operations. Maybe worth revisiting though, if the code can be cleaned up. 


================
Comment at: llvm/lib/TextAPI/Symbol.cpp:68
   SymbolFlags RHSFlags = O.Flags;
-  if ((!O.isData() && !O.isText()) || (!isData() && !isText())) {
-    RemoveFlag(*this, LHSFlags);
-    RemoveFlag(O, RHSFlags);
-  }
+  // Ignore Text and Data for now.
+  RemoveFlag(*this, LHSFlags);
----------------
zixuw wrote:
> Could you explain more about this change? Is this change related to SymbolSet?
I needed it to pass tests. This was an attempt to still check a TBD-v5 only attribute against older TBD file formats. Its not very useful or sound so I just removed it. The lambda should be removed too though. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149860