View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0000362 | LDMud 3.3 | LPC Compiler/Preprocessor | public | 2005-02-02 04:19 | 2018-01-29 21:57 |
| Reporter | Gnomi | Assigned To | |||
| Priority | normal | Severity | minor | Reproducibility | always |
| Status | closed | Resolution | fixed | ||
| Platform | i686 | OS | Debian GNU/Linux | OS Version | 3.0 |
| Product Version | 3.3 | ||||
| Fixed in Version | 3.3.713 | ||||
| Summary | 0000362: Problems with private functions | ||||
| Description | Hi, I looked through my mails for correspondence about this "public fun() shadows private fun()" warning, but came across another bug report and was curious enough to give it a try with the 3.3 driver. This was fixed in 3.2.9-dev.364/365: If a program A has a private function F, that is also the name of a simul-efun, and program B inherits A, it can't call the simul-efun anymore but gets the compiler error 'Function F is private'. The problem I wanted to report (and where I thought, we talked about this in the 3.2 line, but I couldn't find any mails regarding this) is, that private functions are not entirely invisible to inheriting programs (but in my opinion they should be). If the inheriting program has a function that happens to have the same name, it gets a warning 'public programA::fun() shadows private programB::fun()'. This is especially a problem, when we introduce new private functions in the base lib and don't want to grep the whole MUD for it. If you disagree maybe a pragma to turn this warning off would be an option? Greetings, Gnomi. | ||||
| Tags | No tags attached. | ||||
| Attached Files | prolang.privefun.diff (628 bytes)
Index: prolang.y
===================================================================
--- prolang.y (Revision 22)
+++ prolang.y (Arbeitskopie)
@@ -15084,7 +15084,8 @@
/* Handle the non-lfun aspects of the identifier */
{
- if ((n != I_GLOBAL_FUNCTION_OTHER || p->u.global.efun < 0)
+ if (n != I_GLOBAL_FUNCTION_OTHER
+ || (p->u.global.efun < 0 && p->u.global.sim_efun < 0)
|| (fun.flags & (TYPE_MOD_PRIVATE|NAME_HIDDEN)) == 0
|| (fun.flags & (NAME_UNDEFINED)) != 0
)
prolang.privinh.diff (10,524 bytes)
Index: src/prolang.y
===================================================================
--- src/prolang.y (Revision 22)
+++ src/prolang.y (Arbeitskopie)
@@ -2882,8 +2882,8 @@
funp = FUNCTION(num);
- if ((funp->flags & (NAME_INHERITED|TYPE_MOD_PRIVATE))
- == (NAME_INHERITED|TYPE_MOD_PRIVATE))
+ if ((funp->flags & (NAME_INHERITED|TYPE_MOD_PRIVATE|NAME_HIDDEN|NAME_UNDEFINED))
+ == (NAME_INHERITED|TYPE_MOD_PRIVATE|NAME_HIDDEN))
{
break;
}
@@ -14876,28 +14876,12 @@
*/
switch (0) {
default:
- /* Test if the function is visible at all.
- * For this test, 'private nomask' degenerates to 'private'
- * if we didn't do that, the driver would crash on a second
- * level inherit (possible on a multiple second-level inherit).
- * TODO: Find out why it crashes.
+ /* Ignore cross defines.
+ * They are the only complete invisible entries.
*/
- {
- funflag_t fflags = fun.flags;
+ if (fun.flags & NAME_CROSS_DEFINED)
+ break;
- if ((fflags & (TYPE_MOD_PRIVATE|TYPE_MOD_NO_MASK))
- == (TYPE_MOD_PRIVATE|TYPE_MOD_NO_MASK)
- )
- fflags &= ~(TYPE_MOD_NO_MASK);
-
- if ( (fflags & (NAME_HIDDEN|TYPE_MOD_NO_MASK|NAME_UNDEFINED) ) ==
-
- (NAME_HIDDEN|TYPE_MOD_NO_MASK) )
- {
- break;
- }
- }
-
/* Visible: create a new identifier for it */
p = make_global_identifier(get_txt(fun.name), I_TYPE_GLOBAL);
if (!p)
@@ -14955,7 +14939,6 @@
}
else if ((fun.flags | type) & TYPE_MOD_VIRTUAL
&& OldFunction->flags & TYPE_MOD_VIRTUAL
- && !((fun.flags | OldFunction->flags) & NAME_HIDDEN)
&& get_function_id(from, i)
== get_function_id(INHERIT(OldFunction->offset.inherit).prog
, n - INHERIT(OldFunction->offset.inherit).function_index_offset
@@ -14972,104 +14955,70 @@
cross_define( OldFunction, &fun
, n - current_func_index );
}
+ else if ( (fun.flags & (TYPE_MOD_PRIVATE|NAME_HIDDEN|NAME_UNDEFINED))
+ == (TYPE_MOD_PRIVATE|NAME_HIDDEN) )
+ {
+ /* There is already one function with this
+ * name. Ignore the private one, as we
+ * only need it for useful error messages.
+ */
+
+ break;
+ }
+ else if ( (OldFunction->flags & (TYPE_MOD_PRIVATE|NAME_HIDDEN|NAME_UNDEFINED))
+ == (TYPE_MOD_PRIVATE|NAME_HIDDEN) )
+ {
+ /* The old one was invisible, ignore it
+ * and take this one.
+ */
+
+ p->u.global.function = current_func_index;
+ }
else if ( (fun.flags & OldFunction->flags & TYPE_MOD_NO_MASK)
- && !( (fun.flags|OldFunction->flags) & (TYPE_MOD_PRIVATE|NAME_UNDEFINED) ) )
+ && !( (fun.flags|OldFunction->flags) & NAME_UNDEFINED ) )
{
yyerrorf(
"Illegal to inherit 'nomask' function '%s' twice",
get_txt(fun.name));
}
else if (( fun.flags & TYPE_MOD_NO_MASK
- || OldFunction->flags & (NAME_HIDDEN|NAME_UNDEFINED|TYPE_MOD_PRIVATE))
- && !(fun.flags & (NAME_HIDDEN|NAME_UNDEFINED))
+ || OldFunction->flags & NAME_UNDEFINED )
+ && !(fun.flags & NAME_UNDEFINED)
)
{
/* This function is visible and existing, but the
* inherited one is not, or this one is also nomask:
* prefer this one one.
*/
- if (OldFunction->flags & TYPE_MOD_PRIVATE)
- {
- string_t * oldFrom = NULL;
-
- if (OldFunction->flags & NAME_INHERITED)
- {
- oldFrom = INHERIT(OldFunction->offset.inherit).prog->name;
- }
-
- warn_function_shadow( from->name, fun.name
- , oldFrom, OldFunction->name
- );
- }
-
cross_define( &fun, OldFunction
, current_func_index - n );
p->u.global.function = current_func_index;
}
- else if ( (fun.flags & TYPE_MOD_PRIVATE) == 0
- || (OldFunction->flags & TYPE_MOD_PRIVATE) == 0
- || ((OldFunction->flags|fun.flags)
- & TYPE_MOD_VIRTUAL) != 0
- )
+ else
{
/* At least one of the functions is visible
* or redefinable: prefer the first one.
- * TODO: The whole if-condition is more a kludge,
- * TODO:: developed iteratively from .367
- * TODO:: through .370. It should be reconsidered,
- * TODO:: which of course implies a deeper
- * TODO:: analysis of the going-ons here.
*/
- /* Warn about private <-> public collisions;
- * however public <-> public and private <->
- * private are to be expected.
- */
- string_t * oldFrom = NULL;
-
- if (OldFunction->flags & NAME_INHERITED)
- {
- oldFrom = INHERIT(OldFunction->offset.inherit).prog->name;
- }
-
- if ((fun.flags & TYPE_MOD_PRIVATE)
- && !(OldFunction->flags & TYPE_MOD_PRIVATE))
- {
- warn_function_shadow( oldFrom, OldFunction->name
- , from->name, fun.name
- );
- }
- else if (!(fun.flags & TYPE_MOD_PRIVATE)
- && (OldFunction->flags & TYPE_MOD_PRIVATE)
- )
- {
- warn_function_shadow( from->name, fun.name
- , oldFrom, OldFunction->name
- );
- }
-
cross_define( OldFunction, &fun
, n - current_func_index );
}
} /* if (n < first_func_index) */
- else if ( !(fun.flags & NAME_CROSS_DEFINED) )
+ else if ( (fun.flags & (TYPE_MOD_PRIVATE|NAME_HIDDEN|NAME_UNDEFINED))
+ != (TYPE_MOD_PRIVATE|NAME_HIDDEN) )
{
/* This is the dominant definition in the superclass,
* inherit this one.
*/
#ifdef DEBUG
- /* The definition we picked before should be
- * cross-defined to the definition we have now; or
- * it should be nominally invisible so we can redefine
- * it.
+ /* The definition we picked before can't be
+ * cross-defined, because cross-defines won't
+ * be registered as global identifiers.
+ * So the previous definition should be
+ * nominally invisible so we can redefine it.
*/
- if (( !(FUNCTION(n)->flags & NAME_CROSS_DEFINED)
- || FUNCTION(n)->offset.func
- != MAKE_CROSSDEF_OFFSET(((int32)current_func_index) - n)
- )
- && ((FUNCTION(n)->flags & TYPE_MOD_PRIVATE) == 0
- )
- )
+ if ( (FUNCTION(n)->flags & (TYPE_MOD_PRIVATE|NAME_HIDDEN|NAME_UNDEFINED))
+ != (TYPE_MOD_PRIVATE|NAME_HIDDEN) )
{
fatal(
"Inconsistent definition of %s() within "
@@ -15363,6 +15312,16 @@
&& (*flagp & INHERIT_MASK) == inheritc )
{
funp->offset.inherit = inherit_index;
+
+ if(funp != funp2)
+ {
+ /* I don't think, that this is a wise idea.
+ Because the function index offset of this
+ inherit is not correct for this function.
+ */
+ yywarnf("Adjusting inherit index because of virtual inherit %.*s!\n",
+ (int) from->name->size, from->name->txt);
+ }
}
flagp++;
}
prolang.privinh.txt (3,274 bytes)
The first hunk is in define_new_function(). This if-statement ignores any inherited private function, so that it's not redefined. But it also ignores any regular function, that was inherited with the "private functions" modifier (which makes the function not private for us, but to outsiders). That's why I also require NAME_HIDDEN to be set. But NAME_HIDDEN is also set, when it was only a prototype und so I check, that NAME_UNDEFINED is not set. Now to the changes in copy_functions(): This function ignored any NAME_HIDDEN, TYPE_MOD_NO_MASK, but not NAME_UNDEFINED and not TYPE_MOD_PRIVATE functions. As NAME_HIDDEN are either private functions, prototypes or cross-definitions, it can only be later, because private functions have TYPE_MOD_PRIVATE (and "private nomask" becomes "private", so TYPE_MOD_NO_MASK isn't satisfied anymore) and Prototypes have NAME_UNDEFINED. I replaced this by a check for NAME_CROSS_DEFINED. (Don't know, why only nomask cross-definitions are ignored, I think, it should ignore all cross-definitions.) Btw. if the "'private nomask' degenerates to 'private'"-part wouldn't be there, it would also ignore 'nomask private' functions und therefore would not cross-define them, if they were virtual functions. (Why this missing cross-definition crashes, I'll explain later in the part for copy_variables.) virtual private functions should also be cross-defined, that's why I removed the condition for NAME_HIDDEN. (This condition was superflous, because the last else-if-statement would cross-define them anyway.) If this function is private, we keep the first definition. If the previous function was private (TYPE_MOD_PRIVATE, NAME_HIDDEN because of "private functions inherit" making the function also TYPE_MOD_PRIVATE, and not NAME_UNDEFINED to get no prototypes, which are also NAME_HIDDEN), we take the new function. (For the current function a check for TYPE_MOD_PRIVATE would only be required, because it doesn't has the "private functions inherit"-problem. But to make it uniform, I decided to use the same condition for fun and OldFunction.) Only then (just before the cross-definitions) there comes the check for double inheriting 'nomask' functions. Because private functions were handled before, I removed the check for TYPE_MOD_PRIVATE. The same for the following checks: I removed the checks for private functions. So the last if-condition can be omitted completely (we handled private and virtual functions before). After that comes the handling of two functions in the same superclass having the same name. As cross-definitions are handled before, the original if-statement can be left out. But now we have to check whether this new function is really the dominant definition and not just some invisible function. The last part is about copy_variables() and why it crashes if two occurrences of the same private virtual function are not cross-defined. copy_variables would then adjust the inherit index of the second occurrence, but neither the function index nor the inherit function index offset can't be adjusted and so the wrong function index in the inherit's program will be computed and thus lead to a crash. I inserted a warning (because I maybe wrong), but I think it should be a fatal error. Greetings, Gnomi. prolang.privvirtinh.diff (762 bytes)
diff -Naur 3.3privinh/src/prolang.y 3.3privvirtinh/src/prolang.y
--- 3.3privinh/src/prolang.y 2005-08-18 15:14:05.000000000 +0200
+++ 3.3privvirtinh/src/prolang.y 2005-11-30 23:21:00.000000000 +0100
@@ -14951,7 +14958,8 @@
*/
OldFunction->flags |= fun.flags &
(TYPE_MOD_PUBLIC|TYPE_MOD_NO_MASK);
- OldFunction->flags &= fun.flags | ~TYPE_MOD_STATIC;
+ OldFunction->flags &= fun.flags |
+ ~(TYPE_MOD_STATIC|TYPE_MOD_PRIVATE|TYPE_MOD_PROTECTED|NAME_HIDDEN);
cross_define( OldFunction, &fun
, n - current_func_index );
}
| ||||
|
|
I am not sure you want that. Consider this setup: -- a.c --- private sub() { debug_message("a::sub()\n"); } void main() { sub(); } -- b.c --- public sub() { debug_message("b::sub()\n"); } -- c.c --- inherit "a"; inherit "b"; ------ load_object("c")->main() will now print "b::sub()", not "a::sub()", and without the warning ("public b::sub() shadows private a::sub()") you'd have a hard time finding this. Is this a flaw in the way the driver handles inherits? Maybe, but as far as I can see this has been the official behavior since the times of yore (3.2.1): else if (( fun.flags & TYPE_MOD_NO_MASK || OldFunction->flags & (NAME_HIDDEN|NAME_UNDEFINED|TYPE_MOD_PRIVATE)) && !(fun.flags & (NAME_HIDDEN|NAME_UNDEFINED)) ) { /* This function is visible and existing, but the * inherited one is not, or this one is also nomask: * prefer this one one. */ if (OldFunction->flags & TYPE_MOD_PRIVATE) { string_t * oldFrom = NULL; if (OldFunction->flags & NAME_INHERITED) { oldFrom = INHERIT(OldFunction->offset.inherit).prog->name; } warn_function_shadow( from->name, fun.name , oldFrom, OldFunction->name ); } cross_define( &fun, OldFunction , current_func_index - n ); p->u.global.function = current_func_index; } |
|
|
But the following setup has a contrary behavior: -- a.c --- private void sub() { debug_message("a::sub()\n"); } void main() { sub(); } -- b.c --- inherit "a"; public void sub() { debug_message("b::sub()\n"); } load_object("b")->main() will print "a::sub()", so I think, the behavior you describe is a bug in the inheritance handling. (It is not intuitive and - looking at both examples - inconsistent.) Why should one want this behaviour? Greetings, Gnomi |
|
|
Oh, I don't think anybody ever explicitely 'wanted' that behavior - it just happened. |
|
|
Okay, I rephrase. ;-) What kind of code could possibly take advantage of this behavior? (If it is the official behavior, but nobody uses it, a change is not a problem. If it is used, I'd like to see that code. Or better not...) |
|
|
I agree with Gnomi, I checked our Mudlib about this issue and I did not find any advantage or use of this 'feature' so I would prefer to change the inheritance as soon as possible so that in lars' example load_object("c")->main() STILL prints "a::sub()" as it is supposed to be. |
|
|
I attached a diff that solves the first problem (private function overrides a simul-efun). |
|
|
I uploaded a second diff for the 'private functions are not entirely invisible' problem and an explanation for the changes. The diff is pretty experimental, because I can't say I understood all aspects of the code, but I tested it in our TestMUD and it didn't cause a crash. |
|
|
I uploaded a third diff (prolang.privvirtinh.diff) which fixes a problem with a virtual inherit that is inherited twice - one of which with 'private functions'. The driver didn't calculate the best visibility of both occurrences correctly. If one occurrence is private, the function will be private in the program, although the second occurrence may have normal visibility. The posted diff fixes this. |
|
|
It took me longer that it should, but I have incorporated the diffs now. |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2005-02-02 04:19 | Gnomi | New Issue | |
| 2005-02-02 22:34 |
|
Note Added: 0000324 | |
| 2005-02-03 01:34 | Gnomi | Note Added: 0000325 | |
| 2005-02-03 09:20 |
|
Note Added: 0000326 | |
| 2005-02-03 09:30 | Gnomi | Note Added: 0000327 | |
| 2005-03-02 11:39 | Bardioc | Note Added: 0000352 | |
| 2005-08-15 14:17 | Gnomi | File Added: prolang.privefun.diff | |
| 2005-08-15 14:18 | Gnomi | Note Added: 0000381 | |
| 2005-08-18 06:41 | Gnomi | File Added: prolang.privinh.diff | |
| 2005-08-18 06:41 | Gnomi | File Added: prolang.privinh.txt | |
| 2005-08-18 06:52 | Gnomi | Note Added: 0000382 | |
| 2005-11-30 16:58 | Gnomi | File Added: prolang.privvirtinh.diff | |
| 2005-11-30 17:09 | Gnomi | Note Added: 0000430 | |
| 2005-12-04 21:24 |
|
Status | new => resolved |
| 2005-12-04 21:24 |
|
Fixed in Version | => 3.3.713 |
| 2005-12-04 21:24 |
|
Resolution | open => fixed |
| 2005-12-04 21:24 |
|
Assigned To | => lars |
| 2005-12-04 21:24 |
|
Note Added: 0000443 | |
| 2006-02-28 20:04 |
|
Status | resolved => closed |
| 2010-11-16 09:42 |
|
Source_changeset_attached | => ldmud.git master 050d0f46 |
| 2018-01-29 18:59 |
|
Source_changeset_attached | => ldmud.git master 050d0f46 |
| 2018-01-29 21:57 |
|
Source_changeset_attached | => ldmud.git master 050d0f46 |
