On Tue, Jun 10, 2025 at 5:24 PM Branko Čibej <br...@apache.org> wrote:
> On 10. 6. 25 16:01, Timofei Zhakov wrote: > > On Mon, Jun 9, 2025 at 7:26 PM Branko Čibej <br...@apache.org> wrote: > >> On 9. 6. 25 18:31, rin...@apache.org wrote: >> >> Author: rinrab >> Date: Mon Jun 9 16:31:15 2025 >> New Revision: 1926293 >> >> URL: http://svn.apache.org/viewvc?rev=1926293&view=rev >> Log: >> Do not include apr_xlate.h into svn_utf.h, but manually wrap the >> SVN_APR_*_CHARSET constants into APR_*_CHARSET. >> >> >> >> -1. You can't presume that this will never change in APR. >> >> Fix the cmake build to handle include paths correctly. This is not that >> fix. Please revert. >> >> -- Brane >> > > I want to make sure that you entirely understand this change. > > SVN_APR_*_CHARSET family has nothing similar with APR_*_CHARSET since they > are being converted into APR constants through the get_apr_xlate_charset() > function. > > This also means we can add our own shortcuts to SVN_APR_*_CHARSET, which I'm > planning to do for UTF-8. > As long as consumers of our library are not passing APR_*_CHARSET to our utf > routines, we are all fine. > > > Yes. "As long as". This stuff has been in our public header since before > Subversion was an ASF project. APR is and always has been part of > Subversion's public interface. > > It doesn't make us dependent on apr's constants. Please withdraw your veto. > > > You're changing a public header in a backward-incompatible way because the > CMake build has broken include paths. > > You don't see anything wrong with that? Looks to me like you got your > priorities wrong. > > -- Brane > > Okay, I see. You are talking about removing this include, not about the whole change, right? I agree that this is wrong for backward compat to remove this from the private header. However, the remaining part (with SVN_APR_*_CHARSET) is correct. It's still an improvement IMO, which we can use later to add new charset shortcuts (like I said for utf8). -- Timofei Zhakov
