On 10. 6. 25 17:32, Timofei Zhakov wrote:
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
<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_*_CHARSETfamily has nothing similar with APR_*_CHARSETsince 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_*_CHARSETto 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).
No, it is not correct, because this is *not* a private header. It's
public interface.
The proper fix would be to put something like this in CMakeLists.txt:
include_directories($<TARGET_PROPERTY:apr::apr,INCLUDE_DIRECTORIES>
$<TARGET_PROPERTY:apr::aprutil,INCLUDE_DIRECTORIES>)
Right now, your changes for utf-8 argument parsing has spilled into our
public API in an incompatible way, just to avoid fixing cmake.
-- Brane
