labath added a comment. In D75929#1924373 <https://reviews.llvm.org/D75929#1924373>, @ikudrin wrote:
> @dblaikie, @labath, as far as I can understand, the patch complies with your > vision. The main difference is that the enum is still intended for internal > use only, but it probably should not go to the public part before the > proposed values are accepted. Anyway, even while the proposal of the combined > index is not approved, I believe that the patch is useful per se because it > allows reading standard index sections and can later be easily extended for > combined indexes. The patch does not restrict that movement. Please, correct > me if I misunderstand anything. Sorry about the delay. It took a while to get this to the top of my stack. Yes, this "complies with my/our vission", but looking at the `UnknownColumnIds` field, I am starting to have second thoughts about that vision. :( Being able to represent "unknown" columns and at the same time mapping all columns to a internal representation makes things a bit awkward. The reason this wasn't a problem for location lists is that you cannot "skip over" an unknown DW_LLE entry -- it terminates the parse right away. However, that is not the case for debug_?u_index, where you can easily ignore unknown columns. In that light, I am wondering if it wouldn't be better after all to stick to the on-disk column numbers internally (but maybe still keep the "unified" v5 enum in the public interface). @dblaikie, what do you make of that? (btw, is there a test case for the "unknown column" code path?) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75929/new/ https://reviews.llvm.org/D75929 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
