[PATCH] D96409: [debug-info] refactor emitDwarfUnitLength

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin added a comment.

In D96409#2575680 <https://reviews.llvm.org/D96409#2575680>, @dblaikie wrote:

> In D96409#2574186 <https://reviews.llvm.org/D96409#2574186>, @ikudrin wrote:
>
>> More in the sense that it limits the flexibility of calling code without any real profit. For example, the caller now cannot create the label in advance.
>
> Any particular benefit to being able to do that, though? I think this is similar to rolling "Lo" into the function - the caller now can't reuse that label, but it doesn't seem especially important/beneficial to be able to reuse it, compared to simplifying the API.

Why should a service class like `MCStreamer` define the architecture of its clients? It is just not its area of responsibility.

>> For instance, `Dwarf5AccelTableWriter::ContributionEnd` was initialized when the object of that class was created. Simple and solid. Now the initialization is postponed until a method of another class, `Dwarf5AccelTableWriter::Header` is called. The tracking is a bit more challenging now.
>
> Fair - though I'm not sure it's a huge impediment. If the label weren't used in the header it'd probably be a bug - one that hide more silently with the code as-is (perhaps the header code would end up using some other label that is never defined/emitted - and then the assembler would produce some error about that invariant being violated).

I agree that the difference is subtle. If I am not able to convince you, well, so be it. Not a big deal. Just a nice theoretical prattling.

>>> It seems pretty tidy to me, and I'm pretty sure we follow this pattern elsewhere in the CodeGen/AsmPrinter code.
>>
>> Could you get an example? I can't remember anything similar.
>
> Let's see what I can find...
>
>   mcdwarf::emitListsTableHeaderStart (& related APIs like emitLoclistsTableHeader)
>   AddressPool::emitHeader
>
> Those are a couple I could find at a glance at least.

Yeah, they seem similar but aren't they merely helper methods for a limited number of well-known callers? `AddressPool::emitHeader` is even `private`. They are not methods in a distant service class, where callers are not known in advance.

In D96409#2576024 <https://reviews.llvm.org/D96409#2576024>, @shchenz wrote:

> Is `std::pair<MCSymbol *, MCSymbol *>MCDwarfLineTableHeader::Emit(` also an example for this? It returns `LineEndSym` to its caller and its caller will emits the label.

Note that `MCDwarfLineTableHeader` is a private class of `MCDwarfLineTable`. So this is also an implementation peculiarity of `MCDwarfLineTable` which does not affect others.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96409