[Lldb-commits] [PATCH] D56462: Change SymbolFile::ParseTypes to ParseTypesForCompileUnit

[Zachary Turner via Phabricator via lldb-commits]

zturner added a comment.

In D56462#1351391 <https://reviews.llvm.org/D56462#1351391>, @clayborg wrote:

> So I like the ability to specify any symbol context to parse all types for. If you pass in a symbol context with only a module filled in, we parse all types. If we pass in a symbol context with only the compile unit filled in (no function), we parse all types in the compile unit. If we pass in a symbol context with a function or block, we parse all types in that function or block. Currently it is only being used for compile units, but I don't think we need to change the API.


I'm actually specifically trying to remove this flexibility in the API because I think it's harmful.  In fact as we speak I'm writing up an RFC to send to the mailing list.  As someone who just went through implementing a new SymbolFile plugin, it is **very difficult** to implement these functions faithfully for arbitrary SymbolContexts, and begs for untested code as well as dead code (as evidenced by this patch, there was a bunch of dead code for combinations of SymbolContext fields that no caller actually cared about).  So nobody currently even needs this anyway.  which means that specifying an arbitrary `SymbolContext` is a case of YAGNI.  If someone comes along one day and needs the ability to parse types in a function or block, they can call `ParseTypesForFunction` followed by `ParseTypesForBlock`.  This way it is simple and easy for the caller to understand how to use the function, and it is equally simple and easy to understand for the implementer to implement the function.  Currently it is complicated for both.  For example, does specifying a `Module` and a `Function` mean to parse all types for that module //or// that function?  Or does it mean to parse all types for that module //and// that function.

I would like to remove the ability to specify arbitrary `SymbolContext` for most of the other SymboFile functions as well (although there are a few where we might still need it).


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

https://reviews.llvm.org/D56462