Hi,

Finally squashed this bug! Sorry Michael, I couldn't wait - I wanted to get some documentation released and needed this fix.

Patch is include in Mantis report
http://bugs.freepascal.org/view.php?id=17276

In summary
----------
Problem was that the check for Results quit the ResolveLink procedure to early. It never got to the recursive looking through modules or packages. I simply had to tweak the Results check.

Remaining issue.
----------------
Links in module topics (eg: system unit documentation) is not resolved. This bug is not caused by my patch - the bug was there all the time, but unknown to me. No wonder DocView could never find documentation on Pos() function - and I thought it was a DocView bug.

The ScanModule call (inside the linear constructor) does call AddTopicPages, but I don't know why they are not added correctly to the link list. If you add "system.writeln(...)" in the AddLabel() method, you will see in the output that things like "#rtl.system.Pos" does not exist. No idea why yet.

Regards,
- Graeme -

No need to apologize; It's great that you found a fix. I didn't find time to work on it, so if you did: perfect....

We'll have to do some more debugging.

I applied the patch.

Michael.

Op 2010-08-26 16:42, Michael Van Canneyt het geskryf:

We'll have to do some more debugging.

The bugs get stranger by the minute. :-)

The topics in system units doesn't resolve, but the topics in typinfo unit does. Is there actually something like a system.pas unit for fpdoc to parse? I know from history that system unit has always been some "special" unit - not like the others. Don't know if this is related though.

I applied the patch.

Thanks.

Regards,
- Graeme -

for fpdoc, the system unit is like any other.

Michael.

In our previous episode, Graeme Geldenhuys said:

I don't understand this patch!??!?!

What is the pos() case meant to solve? If relative link starts with the current modules unit name, then try package.relativelink?

But what if the relative link is to a different unit? I'm not sure, but afaik this will break stuff that worked before ?

Marco van de Voort wrote

What is the pos() case meant to solve?  If relative link starts with the current modules unit name, then try package.relativelink?

Looking at it again... I first created the "if Pos(...) =1 then...else" code - that solved some of the problems. Then with more testing I noticed more unresolved links and with debugging found out that the recursive "loop through all modules, then through all packages" code was never reached. I then tweaked the code with the "if CanWeExit() then" method.

But looking at it now, I think you might be right. Now that the Result is correctly checked for and the recursive lookups are reached, the first "if Pos(...) = 1 then" code might not be needed any more.

But what if the relative link is to a different unit? I'm not sure, but afaik this will break stuff that worked before ?

Have you got an example? I modified the fpGUI Class documentation over and over, to try all kinds of links. With package info, with module info, with no info other than a identifier etc. etc... The resolve link code seem to work pretty well with all the cases I threw at it. I obviously tested with know problem cases from RTL and FCL too. Every unresolved think I found was manually looked up (it was pretty handy dumping the AddLabel() parameter values to a text file to search against.

As you mentioned, I think the only redundant section is maybe the first "if Pos(..) = 1" block.

I'll double check this at work tomorrow.

Op 2010-08-26 21:17, Marco van de Voort het geskryf:

What is the pos() case meant to solve? If relative link starts with the current modules unit name, then try package.relativelink?

I did some tests and compared output files generated. I only found 4 incorrectly resolved links in the current fpdoc version (RTL docs, dos unit). Removing the if pos(..)=1 block fixes the problem.

Other than than, RTL, FCL and fpGUI documentation output was all identical. On a side note, the current version (timed with EpicTimer) was slightly fast because it needs to do less recursive calls, but the difference was only a few milliseconds in each case.

I attached a patch to the original bug report to remove the "if pos(..)=1" check.

http://bugs.freepascal.org/view.php?id=17276

Regards,
- Graeme -

In our previous episode, Graeme Geldenhuys said:

Maybe. But it seems to effectively disable module.identifier resolution (for modulecurrentmodule). So while it may not be the most used functionality, the question is why it must be (effectively) removed.

Other than than, RTL, FCL and fpGUI documentation output was all identical. On a side note, the current version (timed with EpicTimer) was slightly fast because it needs to do less recursive calls, but the difference was only a few milliseconds in each case.

Did you compare html? It has own code for a lot, and you changed this in dglobals, not some dwlinear/ipf only module.

I attached a patch to the original bug report to remove the "if pos(..)=1" check.

I'll look at stuff tonight or somewhere during the weekend.

Could you btw explain the logic behind the canweexit hack? I don't fully understand why underscores are now equal to points. Won't this cause problems for people that use unit names with underscores?

If this is an IPF limitation, why is it done to a global module?

Specially if users are using whole words like generics_collection or something else whose fragments might clash with other identifiers now.

It was never supported in the first place.

There were only 2 supported ways:

- #package.module.identifier
- identifier

module.identifier was never supposed to work.

Michael.

Op 2010-08-27 13:16, Marco van de Voort het geskryf:

Did you compare html?

Yes, but I used fpGUI documentation only. For RTL, FCL I used LaTex and IPF output.

Could you btw explain the logic behind the canweexit hack? I don't fully understand why underscores are now equal to points. Won't this cause problems for people that use unit names with underscores?

It's not a hack, just a more accurate check to know when we must exit the method. Or you don't like the method name - which I think is descriptive of what it does. ;-)

ALinkDest comes in with the format
<something>.<somethingelse>
ResolveLink always returns something, even if the link was unresolved (in which case it returs the package name). So simply checking to see if Result contains a string longer than 0 makes no sense - hence it always exits before it got to the recursive lookup bit. A simple test: place System.Writeln([1-4]) calls before each ResolveLink call. It never reaches the 3rd or 4th iterations.

So why the '.' replacement to '_'? Because ResoveLink returns the link in a underscore format. So we change ALinkDest (initially passed in value) into something in the same format as what ResolveLink returns. Then we check to see if the original string is in the Result variable - if it is, we know we resolved the link successfully, so we can exit.

Any no it's not a IPF thing, and it doesn't affect unit names with underscores - fpGUI used underscores in every unit. Generating Latex, IPF or HTML documentation for fpGUI classes work fine at the moment.

Specially if users are using whole words like generics_collection or something else whose fragments might clash with other identifiers now.

No, it doesn't affect any identifiers I could see. Again, fpGUI uses underscores in all units, and many classes or types have underscores as well. If you know of an issue, please supply and example.

Regards,
- Graeme -

Op 2010-08-27 13:32, Michael Van Canneyt het geskryf:

Even so, I use that format a lot if fpGUI docs, and they resolve 100% now. So "unofficially" there is no problem using them. I'm not sure what Marco is going on about. Marco, maybe you can supply an example of where it's not working for you? (after my last patch is applied)

Regards,
- Graeme -

In our previous episode, Graeme Geldenhuys said:

Yes, but I used fpGUI documentation only. For RTL, FCL I used LaTex and IPF output.

I'll have to digest your and Michael msgs and check all ends first before I can reply in any sane way.

Till now, I fixed that routine also in a drive-by style, so it is time to really dig into it and make sure there are no problems. However this weekend is 2.4.2rc1 building, so it will probably be after.

Any no it's not a IPF thing, and it doesn't affect unit names with underscores - fpGUI used underscores in every unit. Generating Latex, IPF or HTML documentation for fpGUI classes work fine at the moment.

I was more thinking about e.g.

identifier unitname.something
and unit unitname_something

clashing.

Of course dotted unit names will become a problem in time too.

No, it doesn't affect any identifiers I could see. Again, fpGUI uses underscores in all units, and many classes or types have underscores as well. If you know of an issue, please supply and example.

Well, there is no example needed if it is a fundamental problem. Underscore is a legal identifier character, so using it as separate can introduce clashes. As said, dotted unit names will be a can of worms too.

On the other hand, if there are no current problems, then at least it is not acute.