pharo icon indicating copy to clipboard operation
pharo copied to clipboard

[RB]: definesClassVariable: should be renamed to #hasClassVariableNamed:

Open Ducasse opened this issue 2 years ago • 6 comments

Class >> definesClassVariable: aGlobal
	"Return whether the receiver has a class variables (shared variables among its class and subclasses) named: aString"
	<reflection: 'Class structural inspection - Class variable inspection'>
	^ self classVariables includes: aGlobal

and it would be good to have the equivalent of

RBClass >> definesClassVariable: aSymbol
	"Returns whether the class or its superclasses define a shared variable named: aSymbol"

	self realClass isTrait ifTrue: [ ^ false ].
	(self directlyDefinesClassVariable: aSymbol) ifTrue: [ ^ true ].
	^ self superclass notNil
		and: [ self superclass definesClassVariable: aSymbol ]

May be we could have indirectlyDefinesClassVariable:

Ducasse avatar Oct 09 '23 10:10 Ducasse

I checked this and it seems strange that it expects to compare initialized variables (I mean Variable instances). Instead of:

Class definesClassVariable: #ObsoleteClasses.

always results in false because comparing Variable against Symbol

but doing:

Class definesClassVariable: #ObsoleteClasses asClassVariable.

it also always results in false because comparing initialized class Variable against an uninitialized variable.

So is this implementation correct?

hernanmd avatar Oct 09 '23 10:10 hernanmd

General note: definesClassVariable: on RB uses symbols, on the main model we wanted to use objects more, so there is Class>>#definesClassVariableNamed: to check for names.

now for the object case, it only makes sense to check with the "real" Variable


Class definesClassVariable: (Class classVariableNamed: #ObsoleteClasses) "true"

that is, you need to either get it reflectively with the name, as I did here using #classVariableNamed:, or you could alternatively use #lookupVar: on the class.

MarcusDenker avatar Oct 09 '23 11:10 MarcusDenker

I think it does not take superclasses into account as it really asks the question: Does this class define the Variable? Which means, do I find the Variable in the classPool of this class (e.g. to remove it).

Another way to see it: it talks about definition of a Variable, which the Variable knows and it is just one class:

(SmalltalkImage classVariableNamed: #CompilerClass) definingClass

The other question one can ask "if I use this variable name, will it result in a a class variable?". Then you have to take the superclass into account.

The implementation of the second case the can use standard variable lookup:

hasClassVariableNamed: aSymbol
	^ (self lookupVar: aSymbol ) isClassVariable

MarcusDenker avatar Oct 09 '23 11:10 MarcusDenker

This means, the meaning of #definesClassVariable: is very different in RB compared the main model. I have to admit, I like the main model way better.

MarcusDenker avatar Oct 09 '23 11:10 MarcusDenker

Thanks Marcus for the clarification. I found no users of this case outside of RB. So what we do?

hernanmd avatar Oct 09 '23 15:10 hernanmd

I would:

  • rename the method on RB to be called #hasClassVariableNamed:, #directlyDefinesClassVariable: would then be renamed to #definesClassVariableNamed:
  • implement #hasClassVariableNamed: on the main model so both are in sync (with the implementation as shown in the discussion above)

MarcusDenker avatar Oct 10 '23 08:10 MarcusDenker