<feat>(master.c): catchall for author_file/domain_file

[Shea690901]

the current implementation of both functions may return 0 which is no string despite both functions declared as returning a string...

[quixadhal]

This is one of those annoying cases where LPC's quasi-types will break compatibility if you fix this. :)

FluffOS had the annoying habit of initializing ALL variables to 0, regardless of their type. Because of this, many MUDs will use checks against a variable being 0 to see if the result was set or not... and that bad habit led to some people using 0 as a "no result" code.

So, domain_file() will return 0 when a file is not associated with any domain.

It would be nice if LPC had an actual "undef" or "nil" value that could be assigned and returned, but it doesn't. The closest you can come is checking undefinedp(), but that will only work for variables, not for return values.

On Mon, Jan 11, 2016 at 4:40 AM, Shea690901 [email protected] wrote:

the current implementation of both functions may return 0 which is no

string despite both functions declared as returning a string...

You can view, comment on, or merge this pull request online at:

https://github.com/quixadhal/deadsouls/pull/2 Commit Summary

• (master.c): catchall for author_file/domain_file

File Changes

• M lib/secure/daemon/master.c https://github.com/quixadhal/deadsouls/pull/2/files#diff-0 (54)

Patch Links:

• https://github.com/quixadhal/deadsouls/pull/2.patch
• https://github.com/quixadhal/deadsouls/pull/2.diff

— Reply to this email directly or view it on GitHub https://github.com/quixadhal/deadsouls/pull/2.

[Shea690901]

According to 'vm/internal/base/interpret.cc' line 1317/1320, local variables are initialized as const0u meaning: undefined ;) Consequence: you might return an undefined value by returning a local variable which never got ahything assigned… I know, a kludge, but at least it should work…

But why would you wan't to return 'undef' by either master::author_file or master::domain_file? Alas an object belongs to either of the following three:

1. some wizard
2. some domain
3. the lib itself (backbone or what ever name you want assign)

Both functions should return apropriate strings for all three cases… The driver (package/mudlib_stats) even has special cases for objects belonging to the backbome!

[quixadhal]

I was referring to the fact that there's no equivalent "undef" in LPC itself, not that it isn't used by the driver internally. In LPC, there is no way to say:

int foo;

foo = 3; foo = undef;

Most programmers tend to use undef to mean "no valid value", and it's a pretty common use case to check to see if something is undefined to know if it's either safe to use, or doesn't apply to a particular case. Perhaps my database background speaking, but it's very useful to both be able to distinguish if you got a result, and to lie about it later on. :)

In any case, the issue is that existing mudlibs expect 0 to be a synonym for undef in places, and making such a change will break them. As usual, it comes down to how much you care about supporting existing customers, vs. being more correct.

Personally, I think variables should always be initialized to the appropriate "empty" value of their type... so mappings should be ([]), arrays ({}), floats 0.0, integers 0, and strings "". They should also have an undef "bit" set in their internal structure which undefinedp() would reference, rather than relying on a magic constant value. FInally, having an 'undef' keyword which could be assigned to both reset the value to the default initializer and reset the undefinedp() bit to true, would be handy.

But... I also know that would break a lot of existing LPC code, so that's not my call. :)

On Mon, Jan 11, 2016 at 10:22 PM, Shea690901 [email protected] wrote:

According to 'vm/internal/base/interpret.cc' line 1317/1320, local variables are initialized as const0u meaning: undefined ;) Consequence: you might return an undefined value by returning a local variable which never got ahything assigned… I know, a kludge, but at least it should work…

But why would you wan't to return 'undef' by either master::author_file or master::domain_file? Alas an object belongs to either of the following three:

1. some wizard
2. some domain
3. the lib itself (backbone or what ever name you want assign)

Both functions should return apropriate strings for all three cases… The driver (package/mudlib_stats) even has special cases for objects belonging to the backbome!

— Reply to this email directly or view it on GitHub https://github.com/quixadhal/deadsouls/pull/2#issuecomment-170772041.

[Shea690901]

I agree with you on the point of an 'undef' keyword and also the way it should be implemented… But that still doesn't explain your reasoning to return 0 for those two functions…

The deadsouls lib is the only lib I know that has no catch all and returns 0 in most cases… I could understand it if this 0 where somewhere used by the lib itself, but both functions aren't referenced anywhere. And that the driver accepts 0 as return code of those functions is at best an undocumented feature, at worst one could say, especially when looking into the documentation, that it is a bug…

[quixadhal]

For every mudlib that's been released so we can see how it works, there are 12 more that are locked away, running someone's MUD. Since I don't use one, I wouldn't have a personal issue with changing the behavior, but you have to accept that whenever you change an API, you are making an incompatibility.

As in most cases when I see something that might break things, I urge caution because I don't like pulling the rug out from under people. In practice though, people who are using strange and undocumented features are likely staying with the same driver version they've used for decades as well.

On Wed, Jan 13, 2016 at 12:39 PM, Shea690901 [email protected] wrote:

I agree with you on the point of an 'undef' keyword and also the way it should be implemented… But that still doesn't explain your reasoning to return 0 for those two functions…

The deadsouls lib is the only lib I know that has no catch all and returns 0 in most cases… I could understand it if this 0 where somewhere used by the lib itself, but both functions aren't referenced anywhere. And that the driver accepts 0 as return code of those functions is at best an undocumented feature, at worst one could say, especially when looking into the documentation, that it is a bug…

— Reply to this email directly or view it on GitHub https://github.com/quixadhal/deadsouls/pull/2#issuecomment-171375168.

[Shea690901]

Ok! But that's not exactly a change of api, is it? The api documentation clearly states thatnthe signature of both functions is string->string and 0 is clearly not a atring, so one could even argue that this is a bug fix… And the only reaaon I didn't mark it as such was the fact that the driver for some reason in deed accepts this 0.

I think that most if not all MUDs using this 'buggy' version won't notice this change since in all likelyhood they'll stay with their current verison and even if they update their base I don't think most would notice the difference. Further I think that those who would notice the change shouldn't have any problems in correcting their own code if they haven't already applied similar patches: But I think of those new MUD's basing their code on deadsouls… I don't think they should get implementations of such basic functions that aren't api conform ;)