-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Conversation
@@ -57,13 +59,19 @@ class ValueGenerator extends AbstractGenerator | |||
* @var array | |||
*/ | |||
protected $allowedTypes = null; | |||
/** | |||
* Autodetectable constants | |||
* @var ArrayObject |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you using an ArrayObject
and not simply an array?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. My only comment is that I see no compelling reason for using |
property |
performance comparision scripts? |
} | ||
|
||
/** | ||
* Return constnat list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/constnat/constant/
ValueGenerator constant detection
Merged to develop for release with 2.2.0. |
@@ -239,7 +318,7 @@ public function generate() | |||
); | |||
foreach ($rii as $curKey => $curValue) { | |||
if (!$curValue instanceof ValueGenerator) { | |||
$curValue = new self($curValue); | |||
$curValue = new self($curValue, self::TYPE_AUTO, self::OUTPUT_MULTIPLE_LINE, $this->getConstants()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@3axap4eHko Did you use ArrayObject for this line?
I don't understand the performance improvements you claim. Non return by reference methods implemented on StdLib\ArrayObject are invoked.
The only thing I see the constants are shared by the instances created in this line, so the only need I see $this->constants must be provided as reference. Alternative solutions may
- Use PHP native
ArrayObject
- Refactor getConstants for return by reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Constants is a very big array, containing about 1k string values. Let's length of every string length is 10, therefore total size of array in memory would be greater then 10KB for one instance of ValueGenerator, that means for 1 property of generated class. So for 100 classes with 100 properties total size of constant arrays would be 10MB instead of 10KB for reference value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also you should keep in mind of allocating memory for every array in method. That 100 classes with 100 properties would be have a 10000 arrays
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So native ArrayObject
is suitable for this case and Stdlib
version is not required, isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stdlib\ArrayObject is a wrapper for backward version capability and nothing more
For example, we need to get a file like this:
but we cant pass argument
__DIR__
, because this constant will be parsed and replaced to value, and we must quote it 'DIR' . And finally, after generation, we will not get this:generated value will be like it:
Changes:
Add ability to define environment constants by method
Also add ability to define custom constants by methods
property
$constants
must be type ofArrayObject
for memory saving when pass it as argument for recursive generation by reference.