dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land.
In D139379#4015624 <https://reviews.llvm.org/D139379#4015624>, @ayermolo wrote: > In D139379#4015569 <https://reviews.llvm.org/D139379#4015569>, @dblaikie > wrote: > >> In D139379#3972876 <https://reviews.llvm.org/D139379#3972876>, @ayermolo >> wrote: >> >>> In D139379#3972871 <https://reviews.llvm.org/D139379#3972871>, @dblaikie >>> wrote: >>> >>>> Perhaps the change to use accessors could be removed, now that you've used >>>> it to find all the places that needed to be fixed up? (like just using it >>>> for cleanup/temporary purposes, without needing to commit that API change?) >>> >>> I am not sure what other projects are using it, that I missed, or not in >>> llvm-trunk, but are based of it. >> >> It's awkward to convolute the API to ensure a breakage - I think it's best >> to leave it as-is, for the most part. I guess you needed the 32 bit >> accessors so existing code doesn't become UB because of truncation to 32 bit? >> >> Could the code keep the existing member names, provide the wrappers without >> turning the members into an array and needing named index constants, etc, at >> least? (though even then, seems like there's more to it than needed) > > Thanks for circling back to this during holidays. :) > Right, that was my original thought pattern. I am fully open to the idea that > this is not the right approach. :) I don't know that there's enough out of tree usage to worry about forcing a break by changing the API like this - or to include the "get*32" functions. Is this actually an undefined behavior issue (I don't think the implicit conversion would be any more broken than the explicit cast, at least - but can't find the specific wording about defined/undefined behavior off hand at the moment) , or only an attempt to identify places that could benefit from updates to support 64 bit lengths? Eh, still seems weird, but guess it's not that much of a big deal - so let's go with it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139379/new/ https://reviews.llvm.org/D139379 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
