[PATCH] D137164: [LLParser] Handle mixed blockaddress forward references with names and IDs

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D137164#3918030 <https://reviews.llvm.org/D137164#3918030>, @dexonsmith wrote:

> In D137164#3917926 <https://reviews.llvm.org/D137164#3917926>, @MichielDerhaeg wrote:
>
>> In D137164#3915823 <https://reviews.llvm.org/D137164#3915823>, @dexonsmith wrote:
>>
>>> Would it be simpler to define `<` for that case? Or, we could define a custom one to send to the `std::map`:
>>>
>>>   struct CompareValID {
>>>     static bool operator<(const ValID &LHS, const ValID &RHS) {
>>>       if (LHS.Kind != RHS.Kind)
>>>         return LHS.Kind < RHS.Kind;
>>>       return LHS < RHS;
>>>     }
>>>   };
>>
>> That was my initial idea, but if you search for other ForwardRef-related datastructures in LLParser.cpp, you'll see that they solved it this way for all other cases as well.
>> I did it like this to remain consistent.
>
> Hmm. Okay, the pattern followed elsewhere is reasonable. LGTM then.
>
> That said, the pattern of splitting the map seems to add boilerplate and I'm not convinced it's less error-prone than a shared map. @dblaikie, any thoughts? (I.e., would it be better to tidy it up, or is there some value in using the split maps?)

Yeah, at least at a glance, and with your framing here - I tend to agree that all this duplication seems unfortunate & I guess, by the sounds of it, there's more of this duplication pre-existing in other instances that could be cleaned up if we generalized the op<?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137164