Closes #3660. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/3878
-- Commit Summary --
* Remove custom WIN32 compiler macro in favor of _WIN32
-- File Changes --
M m4/geany-mingw.m4 (1)
-- Patch Links --
https://github.com/geany/geany/pull/3878.patch https://github.com/geany/geany/pull/3878.diff
lgtm
@b4n commented on this pull request.
@@ -418,7 +418,7 @@ static unsigned int re_string_context_at (const re_string_t *input, int idx,
#define re_string_skip_bytes(pstr,idx) ((pstr)->cur_idx += (idx)) #define re_string_set_index(pstr,idx) ((pstr)->cur_idx = (idx))
-#ifdef WIN32 +#ifdef _WIN32
Do we *really* want to modify this upstream-upstream dependency? I guess we'd at least need a patch or a special case in the copy script, because this looks overly fragile to me.
@eht16 commented on this pull request.
@@ -418,7 +418,7 @@ static unsigned int re_string_context_at (const re_string_t *input, int idx,
#define re_string_skip_bytes(pstr,idx) ((pstr)->cur_idx += (idx)) #define re_string_set_index(pstr,idx) ((pstr)->cur_idx = (idx))
-#ifdef WIN32 +#ifdef _WIN32
I couldn't find any reference to this in uctags and so assumed, it's our own copy without upstream. And in https://github.com/geany/geany/pull/3660#issuecomment-2121229911 you said basically the same, didn't you?
If I'm not completely wrong, I'd say this is OK except we intend to upstream this code ever again or, as you also said, replace it the gnulib implementation which uctags uses. Though this feels like a whole another story :(.
@elextr commented on this pull request.
@@ -418,7 +418,7 @@ static unsigned int re_string_context_at (const re_string_t *input, int idx,
#define re_string_skip_bytes(pstr,idx) ((pstr)->cur_idx += (idx)) #define re_string_set_index(pstr,idx) ((pstr)->cur_idx = (idx))
-#ifdef WIN32 +#ifdef _WIN32
Why do we have it if uctags isn't using it?
@b4n commented on this pull request.
@@ -418,7 +418,7 @@ static unsigned int re_string_context_at (const re_string_t *input, int idx,
#define re_string_skip_bytes(pstr,idx) ((pstr)->cur_idx += (idx)) #define re_string_set_index(pstr,idx) ((pstr)->cur_idx = (idx))
-#ifdef WIN32 +#ifdef _WIN32
@elextr because the *used* to use it, but replaced it with a different implementation (gnulib's IIUC) and we didn't sync this.
@elextr commented on this pull request.
@@ -418,7 +418,7 @@ static unsigned int re_string_context_at (const re_string_t *input, int idx,
#define re_string_skip_bytes(pstr,idx) ((pstr)->cur_idx += (idx)) #define re_string_set_index(pstr,idx) ((pstr)->cur_idx = (idx))
-#ifdef WIN32 +#ifdef _WIN32
Oh, so it can go away after the next ctags sync?
@techee commented on this pull request.
@@ -418,7 +418,7 @@ static unsigned int re_string_context_at (const re_string_t *input, int idx,
#define re_string_skip_bytes(pstr,idx) ((pstr)->cur_idx += (idx)) #define re_string_set_index(pstr,idx) ((pstr)->cur_idx = (idx))
-#ifdef WIN32 +#ifdef _WIN32
Oh, so it can go away after the next ctags sync?
I didn't sync it because it's a lot of new files, see
https://github.com/universal-ctags/ctags/tree/master/gnulib
I'm not sure if we want all this but of course I could do it.
@elextr commented on this pull request.
@@ -418,7 +418,7 @@ static unsigned int re_string_context_at (const re_string_t *input, int idx,
#define re_string_skip_bytes(pstr,idx) ((pstr)->cur_idx += (idx)) #define re_string_set_index(pstr,idx) ((pstr)->cur_idx = (idx))
-#ifdef WIN32 +#ifdef _WIN32
Now I'm confused, @b4n says uctags _used_ to use gnu regex, but here its importing half a ton of gnulib including regex?
@b4n commented on this pull request.
@@ -418,7 +418,7 @@ static unsigned int re_string_context_at (const re_string_t *input, int idx,
#define re_string_skip_bytes(pstr,idx) ((pstr)->cur_idx += (idx)) #define re_string_set_index(pstr,idx) ((pstr)->cur_idx = (idx))
-#ifdef WIN32 +#ifdef _WIN32
@elextr they changed the bundled implementation (from an old glibc copy to a newer gnulib one), that's all.
@elextr commented on this pull request.
@@ -418,7 +418,7 @@ static unsigned int re_string_context_at (const re_string_t *input, int idx,
#define re_string_skip_bytes(pstr,idx) ((pstr)->cur_idx += (idx)) #define re_string_set_index(pstr,idx) ((pstr)->cur_idx = (idx))
-#ifdef WIN32 +#ifdef _WIN32
Ahh, ok, misunderstood ya. My quick search seemed to suggest that the change to gnulib removes all the `WIN32`s from ctags and only uses `_WIN32`. So this issue "should" go away when that update is done.
@eht16 commented on this pull request.
@@ -418,7 +418,7 @@ static unsigned int re_string_context_at (const re_string_t *input, int idx,
#define re_string_skip_bytes(pstr,idx) ((pstr)->cur_idx += (idx)) #define re_string_set_index(pstr,idx) ((pstr)->cur_idx = (idx))
-#ifdef WIN32 +#ifdef _WIN32
It's ``` 344K geany/ctags/gnu_regex ``` vs ``` 1.4M ctags/gnulib ``` If size matters. Since we use it probably mainly only for Windows, maybe it does not matter.
PR https://github.com/universal-ctags/ctags/pull/3054 introduced the gnulib version and argues that the glibc version is too old, related issues also state that it is slow. I can't verify if this is also true for Geany but I would guess since we use uctags' parsers, it might be.
@elextr's last comment reads like we already agreed on doing "that update"? :D OTOH it might cause quite some work for a small benefit.
@elextr commented on this pull request.
@@ -418,7 +418,7 @@ static unsigned int re_string_context_at (const re_string_t *input, int idx,
#define re_string_skip_bytes(pstr,idx) ((pstr)->cur_idx += (idx)) #define re_string_set_index(pstr,idx) ((pstr)->cur_idx = (idx))
-#ifdef WIN32 +#ifdef _WIN32
No no, I deliberately didn't say "that update" is needed now, but it will be needed when parsers or other ctags code depends on the included gnulib not the previously included regex.
@eht16 commented on this pull request.
@@ -418,7 +418,7 @@ static unsigned int re_string_context_at (const re_string_t *input, int idx,
#define re_string_skip_bytes(pstr,idx) ((pstr)->cur_idx += (idx)) #define re_string_set_index(pstr,idx) ((pstr)->cur_idx = (idx))
-#ifdef WIN32 +#ifdef _WIN32
```sh # git grep -i tagRegexTable ctags/parsers/ ctags/parsers/dosbatch.c:20:static tagRegexTable dosTagRegexTable [] = { ctags/parsers/dosbatch.c:39: def->tagRegexTable = dosTagRegexTable; ctags/parsers/dosbatch.c:40: def->tagRegexCount = ARRAY_SIZE (dosTagRegexTable); ctags/parsers/matlab.c:20:static tagRegexTable matlabTagRegexTable [] = { ctags/parsers/matlab.c:48: def->tagRegexTable = matlabTagRegexTable; ctags/parsers/matlab.c:49: def->tagRegexCount = ARRAY_SIZE (matlabTagRegexTable); ```
If this detection is sufficient, we have two parsers in Geany using the regex library. For me that doesn't really justify putting efforts into switching the library until there is a need for.
@elextr commented on this pull request.
@@ -418,7 +418,7 @@ static unsigned int re_string_context_at (const re_string_t *input, int idx,
#define re_string_skip_bytes(pstr,idx) ((pstr)->cur_idx += (idx)) #define re_string_set_index(pstr,idx) ((pstr)->cur_idx = (idx))
-#ifdef WIN32 +#ifdef _WIN32
So this issue "should" go away when that update is done.
The point of this has perhaps been missed, the OP has been fixed upstream (by the removal of the offending code), so we can just make changes to our copy to fix it in the meantime and do not need to maintain patches.
@b4n commented on this pull request.
@@ -418,7 +418,7 @@ static unsigned int re_string_context_at (const re_string_t *input, int idx,
#define re_string_skip_bytes(pstr,idx) ((pstr)->cur_idx += (idx)) #define re_string_set_index(pstr,idx) ((pstr)->cur_idx = (idx))
-#ifdef WIN32 +#ifdef _WIN32
Yeah OK as uctags doesn't have this code anymore we can modify it as will. So consider my initial comment in this thread moot.
@elextr commented on this pull request.
@@ -418,7 +418,7 @@ static unsigned int re_string_context_at (const re_string_t *input, int idx,
#define re_string_skip_bytes(pstr,idx) ((pstr)->cur_idx += (idx)) #define re_string_set_index(pstr,idx) ((pstr)->cur_idx = (idx))
-#ifdef WIN32 +#ifdef _WIN32
Just to summarise, since we have had several misunderstandings in this thread, resulting in a long conversation on a two line change:
1. for reasons presented above we will likely need to make the upgrade of this part of ctags at some point 2. that would normally mean we need to maintain changes (or upstream them) as queried at the start of the thread 3. but since upstream has already fixed the problem any upgrade will contain a fix, even if its not what we change, so we don't need to keep our two line fix or upgrade quickly 4. so since its a large upgrade making it at leisure when its actually needed is better than rushing it
@b4n conversation resolved?
@b4n commented on this pull request.
@@ -418,7 +418,7 @@ static unsigned int re_string_context_at (const re_string_t *input, int idx,
#define re_string_skip_bytes(pstr,idx) ((pstr)->cur_idx += (idx)) #define re_string_set_index(pstr,idx) ((pstr)->cur_idx = (idx))
-#ifdef WIN32 +#ifdef _WIN32
Yep. It's especially bad that we spent so many words on this because I still don't really understand the urge to get rid of this macro we define in the build system accordingly… but 🤷♂️
@elextr commented on this pull request.
@@ -418,7 +418,7 @@ static unsigned int re_string_context_at (const re_string_t *input, int idx,
#define re_string_skip_bytes(pstr,idx) ((pstr)->cur_idx += (idx)) #define re_string_set_index(pstr,idx) ((pstr)->cur_idx = (idx))
-#ifdef WIN32 +#ifdef _WIN32
I still don't really understand the urge to get rid of this macro we define in the build system accordingly
Well the OP in #3660 was simply to make meson and autotools do the same thing, seems after a long and winding road the implementation of that was to remove autotools setting a non-standard macro, not add that macro to meson.
Merged #3878 into master.
github-comments@lists.geany.org