Bug 40184 - [PATCH] Move property expansion from Property and MacroInstance to the PropertyHelper class
Summary: [PATCH] Move property expansion from Property and MacroInstance to the Proper...
Status: RESOLVED FIXED
Alias: None
Product: Ant
Classification: Unclassified
Component: Core (show other bugs)
Version: 1.7.0
Hardware: PC Linux
: P2 enhancement (vote)
Target Milestone: 1.8.0
Assignee: Ant Notifications List
URL:
Keywords: PatchAvailable
Depends on:
Blocks:
 
Reported: 2006-08-04 05:32 UTC by Stefano Marsili
Modified: 2009-09-01 05:04 UTC (History)
1 user (show)



Attachments
Modified Property, PropertyHelper (10.94 KB, patch)
2006-08-04 05:38 UTC, Stefano Marsili
Details | Diff
Modified Property, MacroInstance and PropertyHelper (19.27 KB, patch)
2006-08-14 06:32 UTC, Stefano Marsili
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stefano Marsili 2006-08-04 05:32:00 UTC
Why: 
  The property task currently does resolve and expand
  the properties read from a property file 
  (with <property file="..."/>) in an unsynchronised way,
  both accessing the Project and PropertyHelper interfaces.
  In a multithreaded situation this could lead to
  unexpected results. It would also be better to have all
  property handling code in the property helper.

Proposed solution:
  The handling of the properties is naturally moved from the
  Property task to the PropertyHelper. This allows easier 
  synchronisation and possibly code reuse. It also lets
  custom property helpers reimplement the exposed methods.

Stefano Marsili
Comment 1 Stefano Marsili 2006-08-04 05:38:04 UTC
Created attachment 18677 [details]
Modified Property, PropertyHelper

** Sources **
PropertyHelper.java:
 - added public setNewInheritedProperty(ns, n, v)
   like setNewProperty, adds a user property only if 
   there isn't one already defined
 - added addNewProperties(ns, props, prefix)
   resolves, expands, prefixes and adds a set of properties 
 - added addNewInheritedProperties(ns, props, prefix)
   resolves, expands, prefixes and adds a set of user properties 

Property.java:
 - removed resolveAllProperties(props), now in PropertyHelper
 - modified addProperties and addProperty to call the 
   corresponding PropertyHelper methods

** Tests **
No additional testcases added.

** Note **
Modified and patched against latest 1.70alpha.

I did this patch a bit quickly. Probably everything 
(interface, comments, implementation) can be improved.
Maybe the public methods added to the PropertyHelper 
should also be added to the Project (and called from 
the Property task)?
(I also noticed that replaceProperties is not synchronized,
I'm not sure but maybe properties could be set while 
replacing, leading to inconsistent results (not in the patch).)

Stefano Marsili
Comment 2 Stefano Marsili 2006-08-14 06:29:13 UTC
I realised that also MacroInstance's expansion of attributes 
(macrodef task) could be moved to the PropertyHelper so that
all evaluation of expressions is centralized (and may also be
reimplemented).

Thank you
Stefano Marsili
Comment 3 Stefano Marsili 2006-08-14 06:32:08 UTC
Created attachment 18713 [details]
Modified Property, MacroInstance and PropertyHelper

Correction, improvements, ideas are welcome.
Stefano Marsili
Comment 4 Stefan Bodewig 2009-09-01 05:04:40 UTC
Property#resolveAllProperties uses PropertyHelper by now (svn revision 578769 ) while the PropertyHelper logic has been modified strong enough to keep the MacroInstance code separate (we most likely do not want to use the same logic for macro attribute expansions as for property expansions).

Using FIXED; but WONTFIX or "INVALID by now" would be as good as resolution types.