Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Patch in FS#3840 is incorrect #3845

Closed
DorpsGek opened this issue May 19, 2010 · 7 comments
Closed

Patch in FS#3840 is incorrect #3845

DorpsGek opened this issue May 19, 2010 · 7 comments
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/)

Comments

@DorpsGek
Copy link
Member

Krille opened the ticket and wrote:

First I'd like to suggest activating the "Request re-open" feature of Flyspray. This could avoid separate follow-up entries like this one.

The part about string_func.h of the patch is incorrect in the way that it makes the prototype of strndup() go away on newer NetBSDs, but it doesn't remove compilation of the strndup implementation in string.cpp. Previously the # define _GNU_SOURCE took care of both. This may be a minor point, but it is one.

In my patch I left out the version check more or less deliberately since NetBSD versions older than 4.0 aren't supported anymore.

So the version check should also be added in string.cpp or it should be another solution, probably something like DEFINE_STRCASESTR does at the bottom of string_func.h. The latter seems to be the best way.

While I'm at nitpicking let me point out that the log message of r19853 is slightly misleading since you (intentionally?) didn't revert the full r19781. The config.lib part is still there and this is good. It's needed to be able to compile directly from svn. While pkgsrc is cool to install finished packages it's unsuitable for development. The absolute path which is used in config.lib has to be patched by pkgsrc anyway since it allows the user to put the whole tree in /usr/pkg somewhere else.

I had wished to comment on this before the changes were committed, but it seems I was too slow.

Reported version: trunk
Operating system: UNIX


This issue was imported from FlySpray: https://bugs.openttd.org/task/3845
@DorpsGek
Copy link
Member Author

Krille wrote:

Well, it seems I was too mad about the changes that I didn't see the "Request re-open" button... :-(


This comment was imported from FlySpray: https://bugs.openttd.org/task/3845#comment8054

@DorpsGek
Copy link
Member Author

Rubidium wrote:

Okay, I'll let you two first "fight" out what the real solution is... but I have to agree with ggergely that adding _GNU_SOURCE isn't really wanted in light of r15275.


This comment was imported from FlySpray: https://bugs.openttd.org/task/3845#comment8056

@DorpsGek
Copy link
Member Author

Krille wrote:

With the enclosed patch, starting from what we already have in r19861, it would be fine with me.

The smallmap_gui.cpp part is unrelated, it fixes a compiler warning.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/3845#comment8058

@DorpsGek
Copy link
Member Author

Rubidium wrote:

That compiler warning is bogus, i.e. the compiler warns for something that's perfectly fine.


This comment was imported from FlySpray: https://bugs.openttd.org/task/3845#comment8059

@DorpsGek
Copy link
Member Author

ggergely wrote:

Ok, this looks fine to me. but while my (not this fine) patch has compiled the strndup in string.c, but that didn't get linked, as the one from the system headers got picked up. isn't it the case? Either case the program linked fine for me, just had no time to try the game on NetBSD yet.


This comment was imported from FlySpray: https://bugs.openttd.org/task/3845#comment8060

@DorpsGek
Copy link
Member Author

Krille wrote:

@rubidium
Sure, the code is correct, it can't happen that the variable is used unitialized. But why not silence that warning? (Had some cool experiments with PostgreSQL today, whose query planner also can't detect everything which is true)

@ggergely
As far as I see from objdump (don't use it often) it is linked. It might be that the dynamic linker decides to ignore the built-in function and use the one from libc.

objdump -t bin/openttd|fgrep strndup
0830be54 g F .text 00000068 strndup

Anyway, it's more correct the way my new patch does it. ;-)


This comment was imported from FlySpray: https://bugs.openttd.org/task/3845#comment8063

@DorpsGek
Copy link
Member Author

Rubidium closed the ticket.

Reason for closing: Implemented

In r19874


This comment was imported from FlySpray: https://bugs.openttd.org/task/3845

@DorpsGek DorpsGek added Core flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) labels Apr 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/)
Projects
None yet
Development

No branches or pull requests

1 participant