Higuoxing added a comment. In D84008#2161461 <https://reviews.llvm.org/D84008#2161461>, @labath wrote:
> In D84008#2161243 <https://reviews.llvm.org/D84008#2161243>, @Higuoxing wrote: > > > In D84008#2160426 <https://reviews.llvm.org/D84008#2160426>, @MaskRay wrote: > > > > > The number of changed tests is large. Is it worth moving the > > > `IO.mapOptional("Length", Unit.Length);` change to a separate patch to > > > make the refactoring more focused? Thanks > > > > > > This patch is intended to make the length field be inferred when emitting > > the .debug_info section. If we move the `IO.mapOption("Length", > > Unit.Length);` change to a separate change, we might not be able to know > > when to infer the length? There are two visitors, `DumpVisitor` which is > > used to emit the .debug_info section and `DIEFixupVisitor` which is used to > > calculate the length field for us. Do you mean that we keep the > > `DIEFixupVisitor` class and remove the `DumpVisitor` class in this patch? > > > I think that should work if you make it so that this other patch comes before > the functional change in this patch. That other patch could change the > encoding to hex (`uint64_t Length;` -> `yaml::Hex64 Length;`) and make it > default to zero (`IO.mapRequired("Length", Unit.Length);` -> > `IO.mapOptional("Length", Unit.Length, 0);`). That should have no functional > change (I think), but allow you to make the changes in all the yaml files. > The visitation stuff could come after that. Thank you @labath, It sounds good to me! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84008/new/ https://reviews.llvm.org/D84008 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
