FWD » Core Development » Conversion Tools

For an application, you can have a abl/src1/oo/Bar.cls and abl/src2/oo/Bar.cls. In conversion, you include only abl/src2/oo/Bar.cls and some class abl/src/oo/Foo.cls which extends (or inherits) from oo.Bar, as an explicit file list.

On PROPATH in p2j.cfg.xml, you have abl/src1, abl/src2 and abl/src. During conversion, FWD will wrongly match on abl/src1/oo/Bar.cls when resolving oo.Bar, even if this file is not included in conversion.

Of course this can be solved by not having abl/src1/oo/Bar.cls at all on disk, but I think we need to fix FWD so that the PROPATH lookup is done only for files included in conversion.

Question:

Should the classes defined in abl folder be mapped in SymbolResolver.WorkArea.propathClassDicts, and those from skeleton should be mapped in SymbolResolver.WorkArea.oo4glCls?

Danut: Please review this change. I'd like if the approach we take here fits with our long term concepts of #7180 and #6082.

Greg Shah wrote:

Danut: Please review this change. I'd like if the approach we take here fits with our long term concepts of #7180 and #6082.

The changes will not affect #7180, so I have no objections. I'll have to make a small change to fit 7255a on the next rebase, but no major problems.

Code Review Task Branch 7255a Revision 15821

The file-cvt-list.txt is nothing something that is built into FWD. It is just a "convention" and one that is actually being deprecated. For this reason, please remove the reference to fil-cvt-list.txt (note the spelling) in SymbolResolver and the reference to file-cvt-list.txt in ScanDriver.

In regard to the change itself, I wonder if we should put an additional protection into this code. Can we limit the failure condition to when the class basename is duplicated?

It might also be the case where in the non-duplicated basename case, we should add the referenced classname to the conversion list as we plan to do in #6082.

Greg Shah wrote:

In regard to the change itself, I wonder if we should put an additional protection into this code. Can we limit the failure condition to when the class basename is duplicated?

I think I agree with this - but we need to also check skeletons which are already implemented in p2j.oo, so if they are not included in conversion, but exist in PROPATH, then the PROPATH entry will not be used. See #9457-18

It might also be the case where in the non-duplicated basename case, we should add the referenced classname to the conversion list as we plan to do in #6082.

Yes, I agree. Eduard: can we add this, too?

Constantin Asofiei wrote:

Greg Shah wrote:

In regard to the change itself, I wonder if we should put an additional protection into this code. Can we limit the failure condition to when the class basename is duplicated?

I think I agree with this - but we need to also check skeletons which are already implemented in p2j.oo, so if they are not included in conversion, but exist in PROPATH, then the PROPATH entry will not be used. See #9457-18

This name duplication with 4gl buildin classes, or with custom classes (src1/oo/Foo.cls and @src2/oo/Foo.cls, both src1 and src2 included in porpath)

It might also be the case where in the non-duplicated basename case, we should add the referenced classname to the conversion list as we plan to do in #6082.

Yes, I agree. Eduard: can we add this, too?

Yes, it could be done. Went paring a file from conversion list it will go recursively in all the referenced classes. Went we encouter a file not present in conversion list, I add in list in ScanDriver. At the end of the scan of files from the conversion list, the referenced file could be added to files parameter in ScanDriver.scan.

Only thing that should be added a method in FileList to allow adding of a new element.

Made the necessary change in 7255a, rev. 15822.

Eduard Soltan wrote:

This name duplication with 4gl buildin classes, or with custom classes (src1/oo/Foo.cls and @src2/oo/Foo.cls, both src1 and src2 included in porpath)

With both; if you have abl/Progress/Lang/Class.cls, then this must not be included in conversion.

Constantin Asofiei wrote:

Eduard Soltan wrote:

This name duplication with 4gl buildin classes, or with custom classes (src1/oo/Foo.cls and @src2/oo/Foo.cls, both src1 and src2 included in porpath)

With both; if you have abl/Progress/Lang/Class.cls, then this must not be included in conversion.

But if abl/Progress/Lang/Class.cls is included in conversion list?

Eduard Soltan wrote:

Constantin Asofiei wrote:

Eduard Soltan wrote:

This name duplication with 4gl buildin classes, or with custom classes (src1/oo/Foo.cls and @src2/oo/Foo.cls, both src1 and src2 included in porpath)

With both; if you have abl/Progress/Lang/Class.cls, then this must not be included in conversion.

But if abl/Progress/Lang/Class.cls is included in conversion list?

If is included, then yes, go ahead and resolve it from that PROAPTH location. The problem is when the class is resolved in the PROPATH, and is not included in conversion.

Constantin Asofiei wrote:

If is included, then yes, go ahead and resolve it from that PROAPTH location. The problem is when the class is resolved in the PROPATH, and is not included in conversion.

Then my changes should address that.

But I was think about something more general.

I have a src/oo/Foo.cls class which inherits from oo.Bar. Also have src1/oo/Bar.cls and src2/oo/Bar.cls. But src1 and src2 are included in propath. In conversion list I just have abl/src/oo/Foo.cls.

How in this situation I should know which reference class to include in the conversion list.

Eduard Soltan wrote:

But I was think about something more general.

I have a src/oo/Foo.cls class which inherits from oo.Bar. Also have src1/oo/Bar.cls and src2/oo/Bar.cls. But src1 and src2 are included in propath. In conversion list I just have abl/src/oo/Foo.cls.

How in this situation I should know which reference class to include in the conversion list.

The behaviour observed in OE is as folows.

It depends from PROPATH order, I have propath set as src1/oo:src2/oo.

src1/oo/Bar.cls

class oo.Bar:
   method public void method1():
      message "Bar1".
   end.
end.

src2/oo/Bar.cls

class oo.Bar:
   method public void method1():
      message "Bar2".
   end.
end.

Test example:

DEFINE VARIABLE rObj AS CLASS oo.Foo NO-UNDO.
rObj = new oo.Foo().
rObj:method1().

The following code will output: Bar1, however if I change propath to src2/oo:src1/oo output will be Bar2.

FWD works the same way, when it creates the propath map in SymbolResolver.createClassMappings and when it encounters a duplicate entry (oo.Bar for example) it will just ignore it.

I added a check to verify is the previous entry is not in the conversion list and the current one is present, then it should update the map entry.

Are you modifying the PROPATH maps in FWD? I don't want to do this, just check at resolution time .

Constantin Asofiei wrote:

Are you modifying the PROPATH maps in FWD? I don't want to do this, just check at resolution time .

Ok, for a duplicate class name, I will store in propath the first class encountered during scaning of the propath sources. just like in 4gl.

I will have a separate map which will store for duplicate class name, the first class that appeard in the conversion list but wasn't included in the propath.

At resolution time, the file present in the second map takes precedence.

Constantin, regarding the parsing of the reference files.

I have a oo.Foo.cls class the inherits from oo.Bar.cls. And I have both files in the conversion list.

./abl/src/oo/Foo.cls
   Lvl01 parse: ./abl/src1/oo/Bar.cls
   Lvl01 DONE:  ./abl/src1/oo/Bar.cls
   Lvl01 parse: ./abl/src/oo/Foo.cls
   Lvl01 DONE:  ./abl/src/oo/Foo.cls
./abl/src1/oo/Bar.cls (already parsed)

And I have the stack trace during the conversion of oo.Foo where it scans and parses the referenced source.

SymbolResolver.addClass(Aast, Aast, Aast, String) line: 4186    
ProgressParser.class_stmt() line: 6505    
ProgressParser.class_def() line: 8698    
ProgressParser.single_block(boolean, boolean) line: 8412    
ProgressParser.block() line: 7547    
ProgressParser.external_proc() line: 7474    
AstGenerator.parse(String, Set<String>) line: 1643    
AstGenerator.processFile(String, List<File>, int) line: 1055    
SymbolResolver.parseHierarchy(String, SymbolResolver$WorkArea) line: 8875    
SymbolResolver.loadClass(String, boolean) line: 4813    
SymbolResolver.loadClass(String) line: 4607    
ProgressParser.user_defined_type_name() line: 11852    
ProgressParser.inherits_clause() line: 11688    

Yes, this I think is from hierarchy. But you can also have def var l1 as oo.Bar, not just inherits.

Constantin Asofiei wrote:

Yes, this I think is from hierarchy. But you can also have def var l1 as oo.Bar, not just inherits.

But in this case is still the same thing.

   Lvl01 DONE:  ./skeleton/oo4gl/Progress/Lang/Object.cls
   Lvl01 parse: ./abl/src/oo/Foo.cls
   Lvl02 parse: | ./abl/src1/oo/Bar.cls
   Lvl01 DONE:  ./abl/src/oo/Foo.cls
./abl/src1/oo/Bar.cls (already parsed)

OK, now remove from conversion list oo.Bar and check what happens - it should be added automatically

Constantin Asofiei wrote:

OK, now remove from conversion list oo.Bar and check what happens - it should be added automatically

Yes, it will appear in the next phases

./abl/src/oo/Foo.cls
   Lvl01 DONE:  ./skeleton/oo4gl/Progress/Lang/Object.cls
   Lvl01 parse: ./abl/src/oo/Foo.cls
   Lvl02 parse: | ./abl/src1/oo/Bar.cls
   Lvl01 DONE:  ./abl/src/oo/Foo.cls

------------------------------------------------------------------------------
Elapsed job time:  00:00:05

------------------------------------------------------------------------------
Post-Parse Fixups
------------------------------------------------------------------------------

./abl/src1/oo/Bar.cls
./abl/src/oo/Foo.cls
Elapsed job time:  00:00:00.861

------------------------------------------------------------------------------
Early Annotations
------------------------------------------------------------------------------

./abl/src1/oo/Bar.cls
./abl/src/oo/Foo.cls
Elapsed job time:  00:00:00.151

------------------------------------------------------------------------------
Gap Analysis Marking
------------------------------------------------------------------------------

./abl/src1/oo/Bar.cls
./abl/src/oo/Foo.cls
Elapsed job time:  00:00:01.008
...

I am wondering what will happen if I have a usage of normal 4gl procedure like run proc1.p.

Eduard Soltan wrote:

I am wondering what will happen if I have a usage of normal 4gl procedure like run proc1.p.

This is not of concern.

Yes, it will appear in the next phases

This is not OK. It needs to be fully parsed by the scanner (Scanning Progress Source (preprocessor, lexer, parser, persist ASTs)).

Constantin Asofiei wrote:

• if a skeleton class is in abl/ folder and included in conversion, and is also in PROPATH, then it will not be found/converted, and the skeleton/ version will be used. This legacy/skeleton class should also be removed from conversion automatically.

I thought that if it is included in the conversion list, then it should be used.

Eduard Soltan wrote:

Constantin Asofiei wrote:

• if a skeleton class is in abl/ folder and included in conversion, and is also in PROPATH, then it will not be found/converted, and the skeleton/ version will be used. This legacy/skeleton class should also be removed from conversion automatically.

I thought that if it is included in the conversion list, then it should be used.

No, things get wacky if we do so. Ioana has changes in #9488 which allows us to convert legacy classes, by enabling a flag in p2j.cfg.xml. But otherwise, skeleton/legacy classes should never be in abl/ folder, and excluding them automatically will make things easier - see the issue described in #9457-18.

Constantin Asofiei wrote:

I thought that if it is included in the conversion list, then it should be used.

No, things get wacky if we do so. Ioana has changes in #9488 which allows us to convert legacy classes, by enabling a flag in p2j.cfg.xml. But otherwise, skeleton/legacy classes should never be in abl/ folder, and excluding them automatically will make things easier - see the issue described in #9457-18.

Actually, sorry, I read what you noted wrong. If is not included in the conversion list, but in PROPATH, then it must not be found. If is included in the conversion list, then convert it. So, you are correct, please disregard previous comment.

Constantin Asofiei wrote:

To summarize the rules we need to follow:

• if oo.Bar exists only once in PROPATH, but not in conversion list, then will be added to conversion automatically
• if oo.Bar is in both src1 and src2, PROPATH has both src1/ and src2/, and only src2/ is in conversion, then src2/ version of oo.Bar will be used
• if oo.Bar is in both src1 and src2, PROPATH has both src1/ and src2/, and both src1/ and src2/ are in conversion, then src1/ version of oo.Bar will be used
• if oo.Bar is in both src1 and src2, PROPATH has both src1/ and src2/, and neither src1/ and src2/ are in conversion, then src1/ version of oo.Bar will added to conversion
• if a skeleton class is in abl/ folder and included in conversion, and is also in PROPATH, then it will not be found/converted, and the skeleton/ version will be used. This legacy/skeleton class should also be removed from conversion automatically.

Am I missing something?

Changes in the current state, should address all cases.

- if oo.Bar is in src1, src2 and src3, PROPATH has both src1/ ,src2/ and src3/, and in conversion there is src2/ and src3/, then src2/ version of oo.Bar will be used (but is depends of the order in which we specified the PROPATH). For example if the PROPATH is src1:src3:src2, then src3/ version of oo.Bar will be used

For this case I am not really sure, because in PROPATH map at entry oo.Bar we will keep just src1/oo/Bar.cls. And I have 2 options:

1) take the first class in PROPATH which also appears in conversion list (PROPATH: src1:src2:src3, so take oo.Bar version from src2).
2) take the first file that appears in the conversion list.
Example of file-cvt-list.txt:

abl/src3/oo/Bar.cls
abl/src2/oo/Bar.cls

Take the version of oo.Bar from src3.

PROPATH always has precedence. It doesn't matter if multiple .cls with the same basename like oo.Bar appear in multiple folders, if only one folder is in PROPATH, then from there. If none is in PROPATH, then 'error - class not found' should happen.

The PROPATH has the ultimate say in what can be found; the conversion list just tells us how what was found in PROPATH should be used (or not).

Constantin Asofiei wrote:

PROPATH always has precedence. It doesn't matter if multiple .cls with the same basename like oo.Bar appear in multiple folders, if only one folder is in PROPATH, then from there. If none is in PROPATH, then 'error - class not found' should happen.

The PROPATH has the ultimate say in what can be found; the conversion list just tells us how what was found in PROPATH should be used (or not).

Then I think we should have another map (propathMiss) that will keep for a duplicate entries of the class name in the propath will keep the file which also appears in the conversion list.
In my example it will keep for entry oo.Bar -> src2/oo/Bar.cls. If we don't specify any oo/Bar in conversion list, then it will be null. This should be done at PROPATH map population, because from conversion list alone we can't infer whether the file is in propath or not.

In SymbolResolver.findFile, if the propath entry is not present in the conversion list, then we take the source from propathMiss. If we can't find a entry for oo.Bar in propathMiss, only then we add the propath entry to the conversion list.

Eduard Soltan wrote:

In SymbolResolver.findFile, if the propath entry is not present in the conversion list, then we take the source from propathMiss. If we can't find a entry for oo.Bar in propathMiss, only then we add the propath entry to the conversion list.

I don't understand why you don't do this only in findFile. Why the need for the additional data structures? The algorithm should be simple: resolve the list of found files in all entries, and then apply the rules.

Constantin Asofiei wrote:

Eduard Soltan wrote:

In SymbolResolver.findFile, if the propath entry is not present in the conversion list, then we take the source from propathMiss. If we can't find a entry for oo.Bar in propathMiss, only then we add the propath entry to the conversion list.

I don't understand why you don't do this only in findFile. Why the need for the additional data structures? The algorithm should be simple: resolve the list of found files in all entries, and then apply the rules.

What happens if I have in conversion list the src2/oo/Bar.cls, but do not have src2 in propath. findFile will search in propath for oo.Bar entry and will get src1/oo/Bar.cls, which is not in the conversion list. I could take the src2/oo/Bar.cls from conversion list, but I don't know if src2 is in propath. In conversion list we store the full path of the files from file-cvt-list.

You have access to the set of conversion files. findFile will have also the name of physical found file. Make sure to do the lookup depending on the conversion's operating system case-(in)sensitive state - there is CaseInsensitiveHashSet in FWD.

So if you have src1/oo/Bar.cls and src2/oo/Bar.cls in conversion list, and only src2/ in PROPATH, then findFile must find src2/. It does not matter if the file in src1/ is on disk - PROPATH knows what is accessible from that location (at the time of parsing) in findFile.

• if oo.Bar is in both src1 and src2, PROPATH has both src1/ and src2/, and neither src1/ and src2/ are in conversion, then src1/ version of oo.Bar will added to conversion

As PROPATH is gives what it can find, then findFile in the case above must not find anything, because we can't decide which one to add.

• findFile gets the list of all results from PROPATH
• it finds first file which is conversion list and returns it
• otherwise, if there is a single match, it uses it and it also adds it to conversion
• otherwise, no file is in conversion list at this point: as there are multiple matches, show an error message because we can't decide which one to use (the PROPATH needs to be fixed!).

Constantin Asofiei wrote:

So if you have src1/oo/Bar.cls and src2/oo/Bar.cls in conversion list, and only src2/ in PROPATH, then findFile must find src2/. It does not matter if the file in src1/ is on disk - PROPATH knows what is accessible from that location (at the time of parsing) in findFile.

Yes, that is right. findFile will make the lookup in propath first, and will get src2/oo/Bar.cls and will look it up in the conversion files where it is also present, so will return true.

I think I got confused a little, this note I mentioned is wrong:

• if oo.Bar is in both src1 and src2, PROPATH has both src1/ and src2/, and neither src1/ and src2/ are in conversion, then src1/ version of oo.Bar will added to conversion

As PROPATH is gives what it can find, then findFile in the case above must not find anything, because we can't decide which one to add.

Propath in SymbolResolver is the following map: private Map<String, String> propathCls = null;, so it will only be populated with oo.Bar -> src1/oo/Bar.cls. And in findFile there is no way I will even know that there is a src2/oo/Bar.cls. Or you suggest to modify the propath to be private Map<String, Set<String>> propathCls = null;?

Eduard Soltan wrote:

And in findFile there is no way I will even know that there is a src2/oo/Bar.cls.

findFile and PROPATH will find anything on disk. But what is on disk must be in PROPATH to be found.

So if you don't have src2/ in PROPATH, then that must not be found by findFile.

Constantin Asofiei wrote:

Eduard Soltan wrote:

And in findFile there is no way I will even know that there is a src2/oo/Bar.cls.

findFile and PROPATH will find anything on disk. But what is on disk must be in PROPATH to be found.

I think that is the source of confusion. As in the current version of trunk, for duplicate class names (oo/Bar) just the first propath appearance is keep in private Map<String, String> propathCls.

Eduard Soltan wrote:

I think that is the source of confusion. As in the current version of trunk, for duplicate class names (oo/Bar) just the first propath appearance is keep in private Map<String, String> propathCls.

But also in OE, if I have src1/ and src2/ in propath, only the first propath entry is taken into account.

/src/oo/Foo.cls

class oo.Foo inherits oo.Bar:
end.

/src1/oo/Bar.cls

class oo.Bar:
   method public void meth1():
      message "src1/oo/Bar.cls".
   end.
end.

/src2/oo/Bar.cls

class oo.Bar:
   method public void meth1():
      message "src2/oo/Bar.cls".
   end.
end.

def var obj as oo.Foo.

obj = new oo.Foo().
obj:methd1().

Will output src1/oo/Bar.

Eduard Soltan wrote:

Eduard Soltan wrote:

I think that is the source of confusion. As in the current version of trunk, for duplicate class names (oo/Bar) just the first propath appearance is keep in private Map<String, String> propathCls.

Yes, you are right, that value for the map should now be a list.

But also in OE, if I have src1/ and src2/ in propath, only the first propath entry is taken into account.

This is correct.

Keep in mind that this findFile needs to do the lookup in all searchList entries, and only after that decide what to return. So, if you have abl/Progress/Lang/Class.cls, then this must never be found, as is part of oo4glCls.

Ok, after this discussion lets recap a bit to not create any more confusion.

• if oo.Bar exists only once in PROPATH, but not in conversion list, then will be added to conversion automatically

• if oo.Bar is in both src1 and src2, PROPATH has both src1/ and src2/, and only src2/ is in conversion, then src2/ version of oo.Bar will be used

• if oo.Bar is in both src1 and src2, PROPATH has both src1/ and src2/, and both src1/ and src2/ are in conversion, then src1/ version of oo.Bar will be used

• if oo.Bar is in both src1 and src2, PROPATH has both src1/ and src2/, and neither src1/ and src2/ are in conversion, then src1/ version of oo.Bar will added to conversion automatically
  (in OE the src1 is taken and in #7255-40 you stated that neither should be taken). I am not sure here which path to take, I am inclined to say that we should got with src1/.

• if a skeleton class is in abl/ folder and included in conversion, and is also in PROPATH, then it will not be found/converted, and the skeleton/ version will be used. This legacy/skeleton class should also be removed from conversion automatically. Here #7255-34 and #7255-46 also contradicts each other, and here I am inclined to go with #7255-46.

Eduard Soltan wrote:

Ok, after this discussion lets recap a bit to not create any more confusion.

• if oo.Bar exists only once in PROPATH, but not in conversion list, then will be added to conversion automatically

Yes.

• if oo.Bar is in both src1 and src2, PROPATH has both src1/ and src2/, and only src2/ is in conversion, then src2/ version of oo.Bar will be used

Yes.

• if oo.Bar is in both src1 and src2, PROPATH has both src1/ and src2/, and both src1/ and src2/ are in conversion, then src1/ version of oo.Bar will be used

Yes.

• if oo.Bar is in both src1 and src2, PROPATH has both src1/ and src2/, and neither src1/ and src2/ are in conversion, then src1/ version of oo.Bar will added to conversion automatically

This ambiguity in the conversion PROPATH needs to be manually fixed for that application. If we encounter such cases, we can't just decide to add src1/ version - instead, WARNING needs to be logged, that the conversion PROPATH needs to be fixed.

(in OE the src1 is taken and in #7255-40 you stated that neither should be taken). I am not sure here which path to take, I am inclined to say that we should got with

I think in OE '.' is in PROPATH. Create a test where '.' is not in PROPATH and check. I would not expect them to override the PROPATH.

• if a skeleton class is in abl/ folder and included in conversion, and is also in PROPATH, then it will not be found/converted, and the skeleton/ version will be used.

Yes, #7255-46 is the way to go; we will have a mode with #9488 which will allow us to convert skeleton/ADE classes.

This legacy/skeleton class should also be removed from conversion automatically. Here #7255-34 and #7255-46 also contradicts each other, and here I am inclined to go with #7255-46.

inking about the original scenario where a customer has OEUnit classes in the application project, I'm no longer sure that this task plus allowing these files to be in abl/ will solve that problem; as we may not have skeletons for some classes, but they may still be part of OEUnit, and our p2j.oo implementation of this skeleton doesn't match what the file on disk defines...

Frankly, if the solution in this task will completely solve the OEUnit problem we have.

But, regardless, we don't want to have classes part of skeleton/ project in abl/ and in the conversion PROPATH - if that exists in oo4glPath, then it must not be found nor convert; log a big warning that this file will be removed from conversion (this is also because FWD does not support duck-typing - where a legacy class is defined in multiple folders).

Constantin Asofiei wrote:

• if oo.Bar is in both src1 and src2, PROPATH has both src1/ and src2/, and both src1/ and src2/ are in conversion, then src1/ version of oo.Bar will be used

Yes.

I some thoughts about this one, shouldn't we remove from the conversion list the src2/oo/Bar.cls? Because later at conversion phase in SymbolResolver.loadConvertedClass it will try to load from class_definitions table the class for ./src2/oo/Bar.cls, the class will not be found and a error will the thrown stopping the conversion altogether.

Note that this fail with trunk.

Eduard Soltan wrote:

I some thoughts about this one, shouldn't we remove from the conversion list the src2/oo/Bar.cls? Because later at conversion phase in SymbolResolver.loadConvertedClass it will try to load from class_definitions table the class for ./src2/oo/Bar.cls, the class will not be found and a error will the thrown stopping the conversion altogether.

Note that this fail with trunk.

I understand - this requires a manual fix to remove the incorrect file. Leave it as is.

Committed on 7255a, rev. 15825.

• you have a typo in remvoeSource. same in remvoeFile and removedConverstionFiles
• 4GL buildin source should read Legacy builtin class %s found in conversion list, but is part of FWD skeletons - this file will be removed from conversion.
• second warning should read Multiple entries found in PROPATH for %s, but none is in conversion list. Please fix the PROPATH or the conversion list.
• the way the static fields are now in ScanDriver, this does not support multi-threading. At least additionalFileList and removedConverstionFiles need to be a concurrent set.
  • I was thinking to pass these fields to SymbolResolver, but they need to be accessed from multiple SymbolResolver instances. I don't have a better idea now.
• File file: should be File file : .
• shouldn't this code 'break' the loop? Because otherwise you remove the referenceFile, but still do result.filename = propathList.get(0);
  

                    if (ScanDriver.remvoeSource(referenceFile))
                    {                     
                       LOG.log(Level.WARNING, "4GL buildin source encoutered in conversion." 
                                + "Source will be removed from the conversion list.");
                    }
  
  

Constantin Asofiei wrote:

• shouldn't this code 'break' the loop? Because otherwise you remove the referenceFile, but still do result.filename = propathList.get(0);
  [...]

No, it should move to the next iteration of the loop, and resolve the entry from oo4glCls.

Constantin Asofiei wrote:

• the way the static fields are now in ScanDriver, this does not support multi-threading. At least additionalFileList and removedConverstionFiles need to be a concurrent set.
  • I was thinking to pass these fields to SymbolResolver, but they need to be accessed from multiple SymbolResolver instances. I don't have a better idea now.

Yes, I agree. I also think we should also synchronize the operations made on iterator iter, even if it is a ListIterator base on Collections.synchronizedList.

Eduard Soltan wrote:

I also think we should also synchronize the operations made on iterator iter, even if it is a ListIterator base on Collections.synchronizedList.

OK.

• in ScanDriver: either synchronize the method or create the sets in the c'tor. As it is now, this is prone to race condition, as the set fields are allowed to be set concurrently.

Greg: please also review.

Otherwise, please get the #9457 app setup (see #9457-39), and test the #9420 scenario with 9457c + 7255a - change the zfile_set.txt to convert only those files, and see what happens. Also try to add a random .cls file to that project, and see if the dependencies are added to conversion list properly. Post at #9457 any concerns.

Code Review Task Branch 7255a Revisions 15822 through 15827

I don't have an objection to the functional changes.

My only concern is that the changes are likely to have an impact on performance. SR.findFile() is exceptionally performance sensitive and the changes to FileList seem very inefficient (every call to FL.addAdditionalFile() or FL.removeFile() makes a copy of the list).

If there is no major impact on performance then I have no concerns.

Greg Shah wrote:

My only concern is that the changes are likely to have an impact on performance. SR.findFile() is exceptionally performance sensitive and the changes to FileList seem very inefficient (every call to FL.addAdditionalFile() or FL.removeFile() makes a copy of the list).

Since files in FileList is stored as a Java array I don't know whether it could be made in more efficient way. But assuming that file names in the list should be unique and list method always returns sorted elements, maybe we could store the files as a TreeSet and in this way the add and remove operations will be performed in O(log(n)).

Constantin Asofiei wrote:

Otherwise, please get the #9457 app setup (see #9457-39), and test the #9420 scenario with 9457c + 7255a - change the zfile_set.txt to convert only those files, and see what happens. Also try to add a random .cls file to that project, and see if the dependencies are added to conversion list properly. Post at #9457 any concerns.

I converted the files from #9420 with 7255a, and the dependencies are added properly to the conversion list (and as expected get code too large error).

Since files in FileList is stored as a Java array I don't know whether it could be made in more efficient way. But assuming that file names in the list should be unique and list method always returns sorted elements, maybe we could store the files as a TreeSet and in this way the add and remove operations will be performed in O(log(n)).

Using a set would greatly help, but importantly the final result should not be rendered into an array until all add/remove operations are complete.

Committed on 7255a, rev. 15829.

Refactor the addAdditionalFiles and removeFiles methods, to get as a parameter a set of files to be added or removed from the array.

• FileList - this code
  

        if (files == null)
        {
           files = new File[0];
        }
  

  
  should just call list()

Fixed review issue. Committed on 7255a, rev. 15830.

Code Review Task Branch 7255a Revisions 15828 through 15830

I'm good with the changes.

7255a was merged to trunk rev. 15852 and archived.

Would the below testcase have been covered by this task?

On Ubuntu, create ./abl/test.p (just message "Hello world!" and lowercase filename) and zfile_set.txt containing:

F ./abl/Test.p

      <parameter name="opsys"            value="WIN32" />  
      <parameter name="winsys"           value="MS-WINDOWS" /> 
      <parameter name="case-sensitive"   value="false" />

It does not file the file. This is creating a problem with #9813.

Your case-sensitivity is wrong - use F ./abl/test.p. I don't recall exactly, but I think it was discussed in another task.

Constantin Asofiei wrote:

Your case-sensitivity is wrong - use F ./abl/test.p. I don't recall exactly, but I think it was discussed in another task.

That is the entire point... that the file should be found because case should be ignored. I found this in #9813, and you pointed to this task in #9813-42

Roger Borrello wrote:

Constantin Asofiei wrote:

Your case-sensitivity is wrong - use F ./abl/test.p. I don't recall exactly, but I think it was discussed in another task.

That is the entire point... that the file should be found because case should be ignored. I found this in #9813, and you pointed to this task in #9813-42

It may not have been resolved, sorry, I don't recall. For now, for Z mode please assume case-sensitive.