labath accepted this revision. labath added a comment. In D70745#1761544 <https://reviews.llvm.org/D70745#1761544>, @mstorsjo wrote:
> In D70745#1761421 <https://reviews.llvm.org/D70745#1761421>, @labath wrote: > > > In D70745#1761392 <https://reviews.llvm.org/D70745#1761392>, @mstorsjo > > wrote: > > > > > > As for the long if-else cascade, the equivalent elf code was recently > > > > refactored to use llvm::StringSwitch. Doing the same here would be nice. > > > > > > A StringSwitch sounds nice. Some of the cases come with extra conditions > > > (header flags and checking section sizes) though - is it best to keep > > > that as is, if the StringSwitch didn't match anything? > > > > > > I don't really know what are the conventions in the COFF world (you > > probably know that better), but I would tend to trust the section flags > > *more* than any name-based deductions. So I'd try to factor this such (and > > this is what I've done in the elf case) to do the section flag checks > > first, and only then fall back to the section names. The checks for the > > size 0 in data vs. bss sections looks weird to me, and I don't know if its > > really needed, but if you think it is, then it's probably best to pull that > > those cases out separately. > > > Some of those special cases were added rather recently (1 year ago), so I'd > rather not touch them. Checking the flags alone doesn't say much for e.g. > "data" sections (that can be anything between actual data and debug info), so > I didn't find much I dared to touch here, but it can at least be made some > amount more readable, in D70778 <https://reviews.llvm.org/D70778>. > > >> As for @amccarth's suggestion on generically checking for truncated forms, > >> I guess that's not doable with a StringSwitch, but would require e.g. > >> creating more of an ad-hoc table, that we could iterate over, checking for > >> both full and truncated matches? Does that sound sensible to you? > > > > If you think it's good to check the truncated name on all sections, then > > yes, some kind of a table would probably be nice. If it's just one or two > > cases, then you can just spell out both forms in the StringSwitch > > (`.Cases(".eh_frame", ".eh_fram", eSectionTypeEHFrame)` > > Ok, going with just a single Cases here. A table and generic logic for > checking truncated forms might be nice, but I don't see a direct need right > now. SGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70745/new/ https://reviews.llvm.org/D70745 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
