On 2025/8/18 13:18, John Naylor wrote:
We split a patch into multiple patches, it's customary include all of
them, since that process may result in unwelcome artifacts to sort
out. (When the first step has architectural questions or change in
behavior, we may treat it as independent, possibly with a separate
thread, but that's not the case here.)
Thanks for the explanation. I thought to make the second patch only after the first patch is pushed. I am new to PostgreSQL contribution, your guidance is very helpful for my future work.
Now I attach the both patch files.
For the second patch, I have tested it manually again. And "make check" test passed.
-# The lines we care about in the source file look like
+# The lines we care about in the source file look like:
These are spurious changes, which we try to avoid.
Updated.
- next if (!m/<a u="([0-9A-F]+)" b="([0-9A-F ]+)"/);
+ if (/^<U([0-9A-Fa-f]+)>\s+((?:\\x[0-9A-Fa-f]{2})+)\s*\|(\d+)/) {
This change in style caused extra whitespace-only churn. That obscures
what the actual changes are.
Updated.
+ # Match lines like: <UXXXX> \xYY[\xYY...] |n, and use only (|0) mappings
This is missing an explanation of why we skip non-zero mappings.
Code-wise, this only matters for the output in the follow-on patch for
2022, but one of these patches needs to include a brief explanation. I
did not like the detailed description that was present in one of the
earlier 2022 patches that told how many characters were flagged a
certain way -- that's irrelevant detail and will likely get out of
date in some future version anyway.
Okay, I kept a neat version of comment now.
+# and n is a flag indicating the type of mapping having
+# a single value of 0.
This seems weird when combined with the logic to filter out non-zero
mappings. We need to think about when and where to show relevant
information.
Updated the comment.
+ next if ($flag ne '0'); # non-0 flags
This comment is just repeating what the code is doing, and it's very
obvious what it's doing.
Removed the useless comment.
BTW, it sounds like your proposed Makefile changes are needed for the
follow-on patch with .map changes to work at all, is that right?
https://www.postgresql.org/message-id/1CA8625F-AA41-4ED2-B60F-E28AC71F37DC@highgo.com
I think that patch could be separate, because the makefile changes are generic to all map files. The current GB18030 patch doesn't depend on that makefile patch at all. The makefile patch just makes build a little bit easier upon map file changes.
Best regards,
--
Chao Li (Evan)
--------------------
HighGo Software Co., Ltd.
https://www.highgo.com/
