Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fix Issue 7322 -- Taking address of deprecated functions isn't refused #1064

Closed
wants to merge 1 commit into from

7 participants

@yebblies
Collaborator

Please add the test cases that incorrectly fail from the bugzilla report.

@9rnsr
Collaborator

@edmccard : Can you merge small fail_compilation tests into one module with TEST_OUTPUT feature?

@klickverbot
Collaborator

@9rnsr: In case of deprecation warnings, this might work, but in general, I'd prefer them to stay as separate tests. You simply can't test if the compiler exits with a failure for multiple tests in one compilation run, and grepping for all of them in the output might cause problems if we ever change how many error messages are displayed. Hm, maybe using multiple -versions might be a bulletproof way to merge multiple fail_compilation tests together? Would need an extension to d_do_test, though.

@9rnsr
Collaborator

Yes, in general case that way will not work, but a deprecation error does not change the semantics of its later code, so compiler should warn all deprecation uses in one compilation. Then, squashing tests to one module with TEST_OUTPUT can represent such dmd's diagnostic output specification.

@edmccard

I'll have to do some more work. I thought it was working in the case "auto x = &y" but it isn't; I should have time to fix it in the next day or two. Then I'll be able to add the test case from the bug report. I will also consolidate the tests and use TEST_OUTPUT.

@edmccard

Sorry for the delay. The issue was not with auto declarations, but with template functions. At the beginning of SymOffExp::castTo (here: edmccard/dmd@4c497d9#L0L1467) was a test:

if (type == t && hasOverloads == 0) 
    return this;

As far as I can tell, this is only reached when template functions are involved? Removing it allows the test case from the bug report, with &rjustify!string, to pass. and it passes the test suite.

Was there a reason to skip the rest of castTo for template functions (or other things where hasOverloads == 0), or is it safe to remove the check?

(Also, the fail_compliation tests have been consolidated to use TEST_OUTPUT)

@donc
Collaborator

Please remove the import of std.string from the test case. Ideally no tests should use Phobos. Removing all the existing ones would be a pain but we should certainly not introduce any new tests involving it. Your output test includes a deprecation warning, that is definitely going to be removed and will change. If you need to, create a module with a function in it with the same signature as what you're using in the test.

@edmccard

Rebased, and changed the template function test to use a top-level function in the test source, instead of from std.string.

@ghost Show outdated diff Hide outdated diff Unknown commented on an outdated diff
test/fail_compilation/fail7322.d
((2 lines not shown))
+TEST_OUTPUT:
+---
+fail_compilation/fail7322.d(25): Deprecation: function fail7322.f1 is deprecated
+fail_compilation/fail7322.d(26): Deprecation: function fail7322.f2 is deprecated
+fail_compilation/fail7322.d(28): Deprecation: function fail7322.f1 is deprecated
+fail_compilation/fail7322.d(29): Deprecation: function fail7322.f2 is deprecated
+fail_compilation/fail7322.d(32): Deprecation: function fail7322.A1.f is deprecated
+fail_compilation/fail7322.d(35): Deprecation: function fail7322.A2.f is deprecated
+fail_compilation/fail7322.d(38): Deprecation: function fail7322.A1.f is deprecated
+fail_compilation/fail7322.d(41): Deprecation: function fail7322.A2.f is deprecated
+fail_compilation/fail7322.d(43): Deprecation: function std.string.rjustify!(string).rjustify is deprecated - Please use std.string.rightJustify instead.
+---
+*/
+
+#line 1
+import std.string;
@ghost
ghost added a note

The import is still here, maybe you forgot to commit before a push -f?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@edmccard

@AndrejMitrovic, looks like that's what I did. Fixed now.

@donc donc commented on the diff
test/fail_compilation/fail7322.d
((46 lines not shown))
+ f4(&f2);
+
+ auto a1 = new A1();
+ void delegate(float) dg1 = &a1.f;
+
+ auto a2 = new A2();
+ void delegate() dg2 = &a2.f;
+
+ auto a3 = new A1();
+ f5(&a3.f);
+
+ auto a4 = new A2();
+ f6(&a4.f);
+
+ auto fp3 = &f7!string;
+ f4(&f7!string);
@donc Collaborator
donc added a note

Sorry for dragging this on so long, but could you please add a test corresponding to Kenji's comment 4. Eg, right here add:

void function(int) shouldbeok = &f1;

which shouldn't generate any new errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@andralex
Owner

ping?

@andralex
Owner

reping - will close in one week

@9rnsr
Collaborator

I added bug 7322 fix in #2130 - deprecated attribute check should be deferred until one of overloaded function is exactly picked up.

@WalterBright
Owner

@9rnsr does that mean this should be close?

@9rnsr
Collaborator
@9rnsr 9rnsr closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 82 additions and 6 deletions.
  1. +8 −6 src/cast.c
  2. +12 −0 src/expression.c
  3. +62 −0 test/fail_compilation/fail7322.d
View
14 src/cast.c
@@ -1464,8 +1464,6 @@ Expression *SymOffExp::castTo(Scope *sc, Type *t)
printf("SymOffExp::castTo(this=%s, type=%s, t=%s)\n",
toChars(), type->toChars(), t->toChars());
#endif
- if (type == t && hasOverloads == 0)
- return this;
Expression *e;
Type *tb = t->toBasetype();
Type *typeb = type->toBasetype();
@@ -1474,8 +1472,7 @@ Expression *SymOffExp::castTo(Scope *sc, Type *t)
// Look for pointers to functions where the functions are overloaded.
FuncDeclaration *f;
- if (hasOverloads &&
- typeb->ty == Tpointer && typeb->nextOf()->ty == Tfunction &&
+ if (typeb->ty == Tpointer && typeb->nextOf()->ty == Tfunction &&
(tb->ty == Tpointer || tb->ty == Tdelegate) && tb->nextOf()->ty == Tfunction)
{
f = var->isFuncDeclaration();
@@ -1510,6 +1507,7 @@ Expression *SymOffExp::castTo(Scope *sc, Type *t)
e = new SymOffExp(loc, f, 0);
e->type = t;
}
+ checkDeprecated(sc, f);
#if DMDV2
f->tookAddressOf++;
#endif
@@ -1520,7 +1518,10 @@ Expression *SymOffExp::castTo(Scope *sc, Type *t)
e = Expression::castTo(sc, t);
}
else
- { e = copy();
+ { FuncDeclaration *f = var->isFuncDeclaration();
+ if (f)
+ checkDeprecated(sc, f);
+ e = copy();
e->type = t;
((SymOffExp *)e)->hasOverloads = 0;
}
@@ -1553,6 +1554,7 @@ Expression *DelegateExp::castTo(Scope *sc, Type *t)
{ int offset;
if (f->tintro && f->tintro->nextOf()->isBaseOf(f->type->nextOf(), &offset) && offset)
error("%s", msg);
+ checkDeprecated(sc, f);
f->tookAddressOf++;
e = new DelegateExp(loc, e1, f);
e->type = t;
@@ -1566,7 +1568,7 @@ Expression *DelegateExp::castTo(Scope *sc, Type *t)
}
else
{ int offset;
-
+ checkDeprecated(sc, func);
func->tookAddressOf++;
if (func->tintro && func->tintro->nextOf()->isBaseOf(func->type->nextOf(), &offset) && offset)
error("%s", msg);
View
12 src/expression.c
@@ -1084,6 +1084,18 @@ Type *functionParameters(Loc loc, Scope *sc, TypeFunction *tf,
arg = arg->implicitCastTo(sc, p->type);
arg = arg->optimize(WANTvalue);
}
+ else if (arg->op == TOKsymoff)
+ {
+ SymOffExp *se = (SymOffExp *)arg;
+ FuncDeclaration *f = se->var->isFuncDeclaration();
+ if (f)
+ f->checkDeprecated(loc, sc);
+ }
+ else if (arg->op == TOKdelegate)
+ {
+ DelegateExp *de = (DelegateExp *)arg;
+ de->func->checkDeprecated(loc, sc);
+ }
}
if (p->storageClass & STCref)
{
View
62 test/fail_compilation/fail7322.d
@@ -0,0 +1,62 @@
+/*
+TEST_OUTPUT:
+---
+fail_compilation/fail7322.d(25): Deprecation: function fail7322.f1 is deprecated
+fail_compilation/fail7322.d(26): Deprecation: function fail7322.f2 is deprecated
+fail_compilation/fail7322.d(28): Deprecation: function fail7322.f1 is deprecated
+fail_compilation/fail7322.d(29): Deprecation: function fail7322.f2 is deprecated
+fail_compilation/fail7322.d(32): Deprecation: function fail7322.A1.f is deprecated
+fail_compilation/fail7322.d(35): Deprecation: function fail7322.A2.f is deprecated
+fail_compilation/fail7322.d(38): Deprecation: function fail7322.A1.f is deprecated
+fail_compilation/fail7322.d(41): Deprecation: function fail7322.A2.f is deprecated
+fail_compilation/fail7322.d(43): Deprecation: function fail7322.f7!(string).f7 is deprecated
+fail_compilation/fail7322.d(44): Deprecation: function fail7322.f7!(string).f7 is deprecated
+---
+*/
+
+#line 1
+void f1(int a) {}
+deprecated void f1(float a) {}
+deprecated void f2() {}
+
+class A1
+{
+ void f(int a) {}
+ deprecated void f(float a) {}
+}
+
+class A2
+{
+ deprecated void f() {}
+}
+
+void f3(void function(float) fp) {}
+void f4(void function() fp) {}
+void f5(void delegate(float) dg) {}
+void f6(void delegate() dg) {}
+
+deprecated void f7(T)() {}
+
+void main()
+{
+ void function(float) fp1 = &f1;
+ void function() fp2 = &f2;
+
+ f3(&f1);
+ f4(&f2);
+
+ auto a1 = new A1();
+ void delegate(float) dg1 = &a1.f;
+
+ auto a2 = new A2();
+ void delegate() dg2 = &a2.f;
+
+ auto a3 = new A1();
+ f5(&a3.f);
+
+ auto a4 = new A2();
+ f6(&a4.f);
+
+ auto fp3 = &f7!string;
+ f4(&f7!string);
@donc Collaborator
donc added a note

Sorry for dragging this on so long, but could you please add a test corresponding to Kenji's comment 4. Eg, right here add:

void function(int) shouldbeok = &f1;

which shouldn't generate any new errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+}
Something went wrong with that request. Please try again.