Hi all.

I was hoping there could be a discussion of a certain feature in FPC, which doesn't work very well for us. Ultimately, I would like to see it gone completely. It's a very small extra-strong syntax check, appearantly implemented to avoid bad code. However, it's not entirely obvious why this is supposed to help.

Please consider this example:

type
  tfoo = class(tbar)
  private
  public
    procedure Frob();
    procedure Wait(delay: integer);
    procedure ReadStuff(buffer: tbuffer; wait: boolean);
  end;

When declaring classes or interfaces, it is specifically checked that the names of methods are NOT the same as _any_ variable name of _any_ other method in the class. This means that the example above will NOT compile.

Now, in Delphi mode this check is not performed, and so the code above will compile without problems.

The reason for keeping this check, as far as I know, is that it's supposed to lead to better code. For instance, in the body of ReadStuff(), the identifier "wait" could be potentially confused between the "boolean" parameter, and the "procedure(delay: integer) of object;".

At least, that's the theory. Of cours, if the parameter "wait" was a "function (delay: integer) of object", there could be some confusion. This is very rare, however - and you have to be careful anyway, because this restriction is not consistent:

var
  wait: boolean;
procedure tfoo.ReadStuff(buffer: tbuffer; wait: boolean);
begin
  // In here, wait could refer either to the parameter, or the global var.
end;

However, it's not confused with the global var, because standard scoping rules apply. Same as with inner functions and such. Also, if you have a unit foo, with a variable bar, you can use "foo.bar" to refer to that specific var. However, "foo.bar" could also refer to the local record "foo" with the field "bar". Again, standard scoping rules solve this problem without incident.

The _real_ problem, however, is that with these very strict checks we essentially only have one namespace for each class/interface. All function names, parameter names AND local variable names share ONE namespace, with the one exception that no check is performed between functions.

We have several examples where good, standard names are used for functions, and then cannot be used in local variables - this is even though they could never be confused in any realistic scenario.

We urge you to rethink the introduction of these checks. It's making it tremendeously difficult for us to change our code to objfpc mode, and removing it would not break a single line of code anywhere. We have to rethink quite many variable and/or function names, because many good names are already taken. This is, in effect, making our code quality slightly worse overall.

What are your thoughts on this?

Even in delphi, I allways use(d) "procedure ReadStuff(aBuffer: tbuffer;
aWait: boolean);" ..

This won't help you, but you asked for thought. And my thoughts are: I like it the way it is.

regards
Burkhard

Hi,

Let me give you an example of a real bug that allow attackers to gain root access using X-server, and I hope you will understand why such checking can be a good thing.

On C you can have a function and a variable with the same name, and the only way that you can make a difference, is to use the "()" signs. So anything without "()" is variable.

Now, on X-server there was the following (bad coding style as well) of (I'm using my memory here)

...
if (owner == getchown) { ...
..

Now the bug was that they did not place () in "getchown", so it looked for a data that on this case was "0" like the owenr that had "0" for root. so if someone arrived to that code position, they would have been able to use the content (that execute things if I remember it right) to execute their own code under root privliges.

I think that what you have talked about allow you to easily avoid such problems. Beside, a non formal way to write parameters is to add "a" so "await" is good in one location and "wait" in another location.

I do agree, that this type of check can be come smarter, but I do see a need for it.

Another good thing in Pascal, is that you have namespaces for everything (almost), so unlike C, you can actually can guess what variable/paramter you are using, if you do it smart enough. ('with a,b,c do' is not a good way imho).

Ido

C'est magnifique, mais ce n'est pas la guerre.

Two Very Bad Things:

* The restriction increases the context sensitivity of the language.

* The restriction increases the number of incompatibilities with Delphi and therefore increases the difficulty of porting existing programs.

The restriction could be subject to a configurable option (perhaps it is already) but, without a configurable option, the restriction should be omitted. If the language is changed to suit the whims of the developers, it might end up as a fine language, but one with no customers.

Reductio ad absurdum: no identifier may have more than one declaration in a single program because the programmer may get confused as to which thing is being referenced.

Chris Cheney

--------------------------------------------
Mr C.J. Cheney, Head of the Network Division
University of Cambridge Computing Service
New Museums Site, Pembroke Street
Cambridge CB2 3QH
tel: 01223 334428

ik wrote

Let me give you an example of a real bug that allow attackers to gain root access using X-server, and I hope you will understand why such checking can be a good thing.

It *can*. But as usual the pros and cons should be weighted properly. Although I usually like tremendous amounts of compiler checks, because they save my time, I have to agree with Christian here. This sort of "syntax" check is far too strict. If at all, it should be a warning and *only* if the types could actually be confused.

Forcing people to use Hungarian Notation, like it was already suggested, to avoid this check doesn't help neither, there are enough examples for that.

(Granted, maybe this is due to the fact, that I always write "self. ..." for an object method/variable, so local names can't get confused in such a way at all.)

Vinzent.

So, compile using Delphi mode. What's the problem ?

The check ensures that the namespace of the class is cleaner than in Delphi. I have had several live examples where FPC has caught a bug in my Delphi code like this.

So it stays in as far as I'm concerned, I think it is a very good check.

At the best, we could provide a command-line switch or directive to disable or enable this check, but I would not recommend .

If the goal is consistency, I'd rather disable this scenario (with an appropriate error message) as well, rather than enable the other scenario.

However, it's not confused with the global var, because standard scoping rules apply. Same as with inner functions and such. Also, if you have a unit foo,

It's not about confusion to the compiler, it's about confusion to the reader of the code :-).

Micha

What do you mean? It thought it reduces the context sensitivity, because the context is less important to determine what a identifier means. But maybe I missed your point.

* The restriction increases the number of incompatibilities with Delphi and therefore increases the difficulty of porting existing programs.

For porting Delphi programs, compile the unit with -Sd.

Vincent

I'm pretty sure he means that no can no longer, for example, copy a function from one class to another, without risking having to rewrite all the parameter names.

> * The restriction increases the number of incompatibilities with Delphi > and therefore increases the difficulty of porting existing programs. For porting Delphi programs, compile the unit with -Sd.

That's true, but it's still irritating for people who want to go from delphi mode to objfpc mode. Particularly since it doesn't help Regards,
Christian Iversen

You have understood me correctly - I would have expressed it a little differently, but not as succinctly.

language restrictions imposed on the basis of implementors' ideas on how people ought to program should be controlled by options.

Regards

Chris

Having same function names as parameter names per se isn't a biggy but the biggest problem is:

TFirst = class
 protected
  FFirst: Integer;
 public
  property First: Integer read FFirst write FFirst;
end;
TTest = class(TFirst)
  FSomething: Integer;
 public
  procedure DoWithFirst(a, First: Integer);
end;
implementation
procedure TTest.DoWithFirst(a, First: Integer);
begin
  First:=a; // ???
  FSomething:=First; // ???
end;

Ofcourse the intention was setting FSomething to value of argument First and setting FFirst to value of First. It CAN be achieved with "FFirst" or "self.First" but it's just a stupid example of how easily you can overlook things especialy if some parts are hidden in another unit somewhere in an ancestor.

Also notice that in mode delphi there's not a single warning about this.

And believe me things like this CAN happen and will take you 3 days to find out...

I name my arguments "aName" since this incident and not because
{$objfpc}...

PS: there are worse cases but I can't remember a better example now

Ales

Ales, there is _no_ confusion here. Normal scope rules ensures that the first line only assigns the param "a" to the param "first". There are no other ways to interpret that.

The second line assigns the param "First" to the private field "FSomething". Again, no problem at all.

This is very much less of a problem in practice, I believe.

Really, perhaps what we would like to check, is that there are no fields of the same name, _with_a_compatible_type_? That would give warnings that are much better, and still allow (non-method-pointer) parameters in one method to be named the same as another method.

Wouldn't that make more sense?

Also notice that in mode delphi there's not a single warning about this.

True - because it's OK to do this. There are no warnings about all sorts of other normal behaviour either :)

Ok.

PS: there are worse cases but I can't remember a better example now

Let me know if you remember them :)

Agreed.
For me FPC should be a practical tool to build applications in a most productive way.
Something like 'friend units' where protected class members are accessible would be useful to avoid the use of 'cracker classes'.

Martin

Ales, there is _no_ confusion here.

Oh believe me there is. Especialy if you're writing just some little overriden method in a class which doesn't even have "property" visible anymore. You don't think about it and bang, error and a very neatly hidden 's not that I don't know what the assignments mean, it's that when you write code in haste (and 99% of the time you do, unless you write new original stuff) you don't think much about it and sometimes do these things.

I'd atleast like to have warning or at minimum hints point these things out.

Here's the better example:

  TTest = class
   protected
    FField: Integer;
   public
    procedure Helper(Field: Integer);
    property Field: Integer read FField write FField;
  end;

{ TTest }

procedure TTest.Helper(Field: Integer);
begin
  with Self do
    Field:=Field;
  Writeln(Self.Field);
  Writeln(Field);
end;

It's rather idiotic but there are implications..

Ales

P.S: the result is:
0
5

Isn't that exactly why multiple modes exist ? For people not wanting to port ?

Micha

Ok, so enforcing different names is good then, that makes code more context insensitive, since identifiers won't suddenly "resolve" to parameters with the same name.

Micha

This is a weak argument. Suppose this were not true, then the language would be ambiguous (or the semantics undefined)! That would be really really bad. And people are not robots you know, following, knowing and applying rules like an exact grammatical rulebook ;-).

Micha

I think I might have misunderstood Ales's argument :)

No, it's more like the other way around. You get sudden name clashes which you didn't have before, because in the other class the field which clashes with your wisely chosen parameter name didn't exist.

There's a reason, I always write "self.Identifier" and I also refuse to revert back to the so called Hungarian notation (like AParameter). Even if there is *no* parameter and/or field with that name it's always clear which part is meant.

It would be more wise to warn about name clashes between units, because for example NewStr is defined in objects.pas and in classes.pas (IIRC), but depending on which one and how it is used your program may crash.

As long as such problems exist in the language definition itself, this whole discussion about "cleaner code" and if such syntactic checks are needed is f*cking pointless, because the language already forces you to use fully qualified names anyway if you want to avoid such problems. So you can as well use "self" to distiguish between parameters and fields even if they have the same name.

Vinzent.

We should be talking about semantics, not about 'compabilility whatever the resulting meaning may be'.

There's a reason, I always write "self.Identifier" and I also refuse to revert back to the so called Hungarian notation (like AParameter). Even if there is *no* parameter and/or field with that name it's always clear which part is meant.

This is not Hungarian notation. Hungarian is to prefix variable names with the (abbreviated) type.

It would be more wise to warn about name clashes between units, because for example NewStr is defined in objects.pas and in classes.pas (IIRC), but depending on which one and how it is used your program may crash.

Agree, it's separate, but a good point.

In theory, yes, but in practice nobody prefixes every field usage of self with 'Self.'.

Micha

Yes, but the semantics is clear due to scoping rules. It may not be obvious, though. ;)

Oh, come on. It's still a meaningless prefix not checked by anyone, at last the compiler. Especially when software gets changed usually the prefixes remain for whatever "historical reason" and the meaning changes (anyone remember LPSZSomething? Long Pointers are gone since ages, but the type names remained for "compatibility"), so those prefixes are much more dangerous than they appear at first sight, if you trust them. And if you don't trust them, that means you check it for yourself and then you can get rid of them, because the serve no purpose other than making variable names totally unreadable.

Well, I don't consider it *that* separate. There's a "scoping" rule defined by the language and as such there are no ambiguities (at compiler level), yet it's not obvious to the developer. Change the order in which units are included and the semantics may change. Of course, the semantic change would still be defined by the language.

I remember trying to compile some TP7 code and I couldn't figure out what went wrong until I noticed there was one unit included that defined a variable called "Break".

Finally finding why

|if condition then
|   break;

didn't compile anymore although it did a couple of days before taught me to always use the full name prefixed by the unit's name.

Not even counting my"self" (and not counting Ada programmers, where similar things are much more common, because there the compiler stops at any *possible* ambiguity), I know at *least* two nobodies then.

And hell, I lost track how often have I seen "this. ..." in highly modern Java code. ;) So it seems much more common than you may think.

Vinzent.

Thankfully I've read recently that this is "hungarian notation done wrong" ;-) It said the guy quoted as being the creator of the "hungarian notation" used prefixes according to use of the variable and/or its scope. It seems he is a smart guy and knew variable type is a compiler thing and we shouldn't be bothered with it ;-)
But then probably one of his colleagues in MS got it totally wrong and started using this "type prefix" thing...

At my workplace we've standardized on using three "scope prefixes": p_ for function parameter, m_ for members/fields, and g_ for global. I have used "scope prefixes" long before this/them and I as a somewhat-die-hard-Pascal-programmer had a hard time getting used to the underscores, which seems to divide the identifier and makes you code look like C, but I barely notice it now ;-) And it's very practical for code completion :-)
Most of the our projects nowadays are written in Java, though.

Cheers,
Flávio

Flávio Etrusco wrote

At my workplace we've standardized on using three "scope prefixes": p_ for function parameter, m_ for members/fields, and g_ for global.

I know that discussion, I lost it at work already, but apart from C where every identifier is sort of global, there's absolutely no need for this:

Writing this/self. removes the need for m_ and even adds some sort of safety, because that's one rule the compiler can actually check. - That's one.

The difference between locals and paranetrized locals does not need to be mentioned at all, because inside the function they behave the same and have the same scope. - That' two.

Globals should be forbidden. That's three.
Hmm, ok, if really needed, you should prefix the unit name to ensure sudden name clashes (like my "Break" identifier) anyway, so globals are easily identified simply by being used already. - So, now that's three.

Well, since my variable names tend to be longer than four chars, I always use underscores and my code sure does NOT look like C. ;)

ItMakesIdentifiersMoreReadableBecauseTheUnderscoreIsAsCloseAsYouCanGetToNormalTextWithoutUsingWhitespace.

Vinzent.