Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Setting BUILTIN_STL_SUPPORT to YES causes function page to not list all classes with Doxygen 1.8.18 and later #10873

Closed
scott-zhong opened this issue May 17, 2024 · 19 comments

Comments

@scott-zhong
Copy link

Describe the bug
Setting BUILTIN_STL_SUPPORT to YES causes function page to not list all classes with Doxygen 1.8.18 and later. NOTE: The total number of function has to exceed the threshold for a single page such that Doxygen breaks out into separate function pages. The page noted in the screenshots are for functions_z.html page.

Screenshots
1.8.17:
image

1.8.18:
image

trunk (b505f2e):
image

To Reproduce
testcase.zip

Expected behavior
ZTx() should list CODec<T,X>, SCDec<T>
Zx() shoudl list CODec<T, X>, SCDec<T>

Additional context
While developing the testcase, removing certain undocumented members or functions from the class would sometime cause the problem to be fixed. This may indicate a potential uninitialized variable or some state not being reset in the Doxygen code.

@scott-zhong
Copy link
Author

Also the function list is not sorted on trunk (b505f2e) -- see the screenshot in the description.

@albert-github albert-github added needinfo reported bug is incomplete, please add additional info C/C++ labels May 18, 2024
@albert-github
Copy link
Collaborator

In the 1.8.18 version it indeed looks like the sorting is not correct, though I see no missing functions compared to 1.8.17

With the current doxygen version and the current master (1.11.0 (c3c2d7e)) I get:

image

and this looks OK to me.
(also with the version 1.11.0 (b505f2e) all is OK)

  • did I miss something?

@doxygen
Copy link
Owner

doxygen commented May 18, 2024

@albert-github

With the same c3c2d7e I get this:
image

i.e. two ZTx() entries. So it looks like there is some unpredictable behavior in the sort or write routine.

@albert-github
Copy link
Collaborator

@doxygen indeed looks like some unpredictable results.

  • I tested on Windows and the results as I reported
  • I just now tested the doxygen 1.10.0 version on Cygwin and I got the wrong result.

@albert-github albert-github added bug and removed needinfo reported bug is incomplete, please add additional info labels May 18, 2024
@albert-github
Copy link
Collaborator

Intriguing problem.

An observation:
I just looked at the function: void Index::sortMemberIndexLists()
and replaced here the qistrcmp with qstrcmp (and added .data() to the arguments, and the functions_z.html looks to be identical between Windows and Cygwin (though I don't like the sorting:
image

as now the zero is ate the end)

Looks like the qstricmp has been used to do some intelligent sorting (to keep uppercase and lowercase names a bit together) and maybe this is not executed everywhere.

There are still a number of other files where similar problems look like to be:

class_simple-members.html
class_traits-members.html
class_zone-members.html
functions_func.html
search/all_1.js
search/all_19.js
search/all_2.js
search/functions_1.js
search/functions_15.js
search/functions_2.js

@doxygen
Copy link
Owner

doxygen commented May 18, 2024

@albert-github Indeed. The index lists are sorted in a case-insensitive way at the moment, but later when writing the entries they are grouped together using a case-sensitive name check against the previous item. If we do all comparison in a case-sensitive manner, you get what you observed, because 'z'>'Z' in the ASCII table. If we go for the case-insensitive approach, the heading of items ZTx() and ZTX() will be merged even though these are different member functions, which is also not ideal. So not sure yet what is the best approach.

@albert-github
Copy link
Collaborator

@doxygen

The merging of ZTx() and ZTX() for C++ is not a good idea better said it is a bad idea (for Fortran it would be perfect though).
The strange thing is of course that there is a difference between the behavior on Windows (which looks OK to me) and the other platforms (at least the ones we now tested) and this should not be the case.

@doxygen
Copy link
Owner

doxygen commented May 18, 2024

@albert-github The difference is probably because lower case compares causes duplicate keys during sorting, in which case std::sort is not predictable. Probably, usingstd::stable_sort should give the same results everywhere (without fixing the problem).

@albert-github
Copy link
Collaborator

In the documentation of std::sort I see (bold mine):

Sorts the elements in the range [first, last) in non-descending order. The order of equal elements is not guaranteed to be preserved.

and for std::stable_sort:

Sorts the elements in the range [first, last) in non-descending order. The order of equivalent elements is guaranteed to be preserved.

I did a quick test (replacing all std::sort by std::stable_sort except in reflist.cpp as I didn't know quickly how it should look like).
The results between the Windows and Cygwin run are identical and it look to me also that they are OK.

I'm a bit worried about the remark between brackets: "without fixing the problem".

Probably we should use std::stable_sort everywhere.

@albert-github
Copy link
Collaborator

Another thought. Based on the qstricmp / gstrcmp idea of #10873 (comment) and the stability idea of stable_sort and the remark "without fixing the problem".

We now have in Index::sortMemberIndexLists() / auto sortMemberIndexList = [](MemberIndexMap &map):

            int result = qstricmp(md1->name(),md2->name());
            return result==0 ? qstricmp(md1->qualifiedName(),md2->qualifiedName())<0 : result<0;

when we would replace this by:

            int result = qstricmp(md1->name(),md2->name());
            if (result==0) result = qstrcmp(md1->name().data(),md2->name().data());
            if (result==0) result = qstricmp(md1->qualifiedName(),md2->qualifiedName());
            return result==0 ? qstrcmp(md1->qualifiedName().data(),md2->qualifiedName().data())<0 : result<0;

results at first glance for the limited test look OK or are we running here possibly in the same problem mentioned:

If we go for the case-insensitive approach, the heading of items ZTx() and ZTX() will be merged even though these are different member functions, which is also not ideal.

in #10873 (comment) as well?

@doxygen
Copy link
Owner

doxygen commented May 18, 2024

@albert-github I came to the same solution as you proposed. Will make a commit for it soon.

doxygen added a commit that referenced this issue May 18, 2024
…to not list all classes with Doxygen 1.8.18 and later
@doxygen
Copy link
Owner

doxygen commented May 18, 2024

@scott-zhong Please verify if the referenced commit fixes the problem for you. Do not close the issue, this will be done automatically when the next official release becomes available.

@doxygen doxygen added the fixed but not released Bug is fixed in github, but still needs to make its way to an official release label May 18, 2024
@albert-github
Copy link
Collaborator

@doxygen

This was not exactly what I had in mind and it is also not 100% correct.

My idea was to replace all the qstricmp in the sort functions by the construct:

   result = qstricmp(...);
   if (result == 0) result = qstrcmp(...);

I man de small test:

/// \file

/// the class
class cls
{
  public:
    /// \details the details
    a(float i) {}
    /// \details the details
    A(float i) {}
    /// \details the details
    A(int i) {}
    /// \details the details
    a(int i) {}
    /// \details the details
    a(double i) {}
    /// \details the details
    A(double i) {}
    /// \details the details
    a() {}
    /// \details the details
    A() {}
};

and settings

QUIET = YES
SORT_MEMBER_DOCS       = YES
SORT_BRIEF_DOCS        = YES
SORT_MEMBERS_CTORS_1ST = NO
SORT_GROUP_NAMES       = NO
SORT_BY_SCOPE_NAME     = NO

Which results (for Windows and Cygwin) into (see classcls.html):
image

Note the order for the int versus the other versions, in my opinion either:

  • order should be consistent so either
    • the int should also be a followed by A
    • all "other" should be A followed by a
  • all lowercase should be together and all uppercase should be together

In general as a has number 97 and A has number 65 in the ASCII table so I would expect A always before a.

Example: example.tar.gz

doxygen added a commit that referenced this issue May 19, 2024
…to not list all classes with Doxygen 1.8.18 and later

More generic fix.
@doxygen
Copy link
Owner

doxygen commented May 19, 2024

@albert-github I agree this should be solved in a more generic way. See new commit.

@albert-github
Copy link
Collaborator

@doxygen

I agree this should solve the problem in a more generic way.

I see a few things, which might be intentionally:

  • in reflist.cpp in the function RefList::generatePage I see: { return qstricmp(left->title(),right->title()) < 0; }); is this on purpos
  • similar in createJavaScriptSearchIndex
  • on a number of places I see that in the past there was already a < used e.g. in htmlhelp.cpp the function HtmlHelpIndex::writeFields(std::ostream &t)

doxygen added a commit that referenced this issue May 19, 2024
…to not list all classes with Doxygen 1.8.18 and later
@doxygen
Copy link
Owner

doxygen commented May 19, 2024

@albert-github Addressed those as well, to be on the safe side.

@albert-github
Copy link
Collaborator

@doxygen

Just listing / for the record the ones that don't use qstricmp_sort.

Very tricky ones (probably on purpose left):

  • in function DefinitionImpl::mergeRefItems in definition.cpp
  • in function parseInput (nearly at the end of the function) in doxygen.cpp
  • in function FileDefImpl::sortMemberLists in filedef.cpp

Very special one (also probaly left on purpose):

  • in function generateJSNavTree in ftvhelp.cpp

@doxygen
Copy link
Owner

doxygen commented May 19, 2024

@albert-github indeed, I think these can stay as they are.

@doxygen
Copy link
Owner

doxygen commented May 20, 2024

This issue was previously marked 'fixed but not released',
which means it should be fixed in doxygen version 1.11.0.
Please verify if this is indeed the case. Reopen the
issue if you think it is not fixed and please include any additional information
that you think can be relevant (preferably in the form of a self-contained example).

@doxygen doxygen removed the fixed but not released Bug is fixed in github, but still needs to make its way to an official release label May 20, 2024
@doxygen doxygen closed this as completed May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants