Skip to content

Commit

Permalink
issue #8752: Generated filenames depend on undefined behaviour - doxy…
Browse files Browse the repository at this point in the history 
…gen output not reproducible
  • Loading branch information
@doxygen
doxygen committed Sep 12, 2021
1 parent 897b44c commit 44e27fa
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 6 deletions.
24 changes: 18 additions & 6 deletions src/dirdef.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ class DirDefImpl : public DefinitionMixin<DirDef>
virtual void setDiskName(const QCString &name) { m_diskName = name; }
virtual void sort();
virtual void setParent(DirDef *parent);
virtual void setDirCount(int count);
virtual void setLevel();
virtual void addUsesDependency(const DirDef *usedDir,const FileDef *srcFd,
const FileDef *dstFd,bool srcDirect, bool dstDirect);
Expand All @@ -92,7 +93,7 @@ class DirDefImpl : public DefinitionMixin<DirDef>
QCString m_shortName;
QCString m_diskName;
FileList m_fileList; // list of files in the group
int m_dirCount;
int m_dirCount = -1;
int m_level;
DirDef *m_parent;
UsedDirLinkedMap m_usedDirs;
Expand All @@ -107,8 +108,6 @@ DirDef *createDirDef(const QCString &path)
//----------------------------------------------------------------------
// method implementation

static int g_dirCount=0;

DirDefImpl::DirDefImpl(const QCString &path) : DefinitionMixin(path,1,1,path)
{
bool fullPathNames = Config_getBool(FULL_PATH_NAMES);
Expand All @@ -132,7 +131,6 @@ DirDefImpl::DirDefImpl(const QCString &path) : DefinitionMixin(path,1,1,path)
m_dispName = m_dispName.left(m_dispName.length()-1);
}

m_dirCount = g_dirCount++;
m_level=-1;
m_parent=0;
}
Expand Down Expand Up @@ -163,6 +161,11 @@ void DirDefImpl::setParent(DirDef *p)
m_parent=p;
}

void DirDefImpl::setDirCount(int count)
{
m_dirCount=count;
}

void DirDefImpl::addFile(const FileDef *fd)
{
m_fileList.push_back(fd);
Expand Down Expand Up @@ -1006,7 +1009,6 @@ void buildDirectories()
{
for (const auto &fd : *fn)
{
//printf("buildDirectories %s\n",qPrint(fd->name()));
if (fd->getReference().isEmpty())
{
DirDef *dir;
Expand Down Expand Up @@ -1051,7 +1053,17 @@ void buildDirectories()
std::sort(Doxygen::dirLinkedMap->begin(),
Doxygen::dirLinkedMap->end(),
[](const auto &d1,const auto &d2)
{ return qstricmp(d1->shortName(),d2->shortName()) < 0; });
{ int i1 = qstricmp(d1->shortName(),d2->shortName());
int i2 = qstricmp(d1->name(),d2->name());
return i1 < 0 ? true : i2<0 ? true : false;

This comment has been minimized.

Sign in to view

Copy link
@j6t

j6t Jan 7, 2022 •
edited
Loading

Please reconsider the implementation of this comparator. I fear it does not satisfy the "strict weak order" precondition of std::sort.

The return statement basically means:

return d1->shortName < d2->shortName || d1->name < d2->name;

Such a straight-forward disjunction is typically inappropriate for std::sort. For example, if you have one value dA with shortName A and name Z/A and another value dB with shortName B and name Y/B, then both dA < dB and dB < dA are true according to this comparator. But this must not be the case for a "strict weak order".

The correct return statement must be

return i1 < 0 || (i1 == 0 && i2 < 0);

This comment has been minimized.

Sign in to view

Copy link
@albert-github

albert-github Jan 7, 2022

Collaborator

The first question is are we looking here on the code of the release or of the commit. We see that this is the later as in the 1.9.3 release I see:

  std::sort(Doxygen::dirLinkedMap->begin(),
            Doxygen::dirLinkedMap->end(),
            [](const auto &d1,const auto &d2)
            {
              QCString s1 = d1->shortName(), s2 = d2->shortName();
              int i = qstricmp(s1,s2);
              if (i==0) // if sort name are equal, sort on full path
              {
                QCString n1 = d1->name(), n2 = d2->name();
                int n = qstricmp(n1,n2);
                return n < 0;
              }
              return i < 0;
            });

Looking in the git blame and repository for commit 0320654 we see that here the comment of the commit is: Regression for issue #8752: Fixed problem with using a temporary reference

So I think the implementation already has been reconsidered and implemented according to the 44e27fa#r63067187

This comment has been minimized.

Sign in to view

Copy link
@j6t

j6t Jan 7, 2022

Thanks, that looks good. I was pointed to this commit via the release notes for 1.9.3.
});

// set the directory count identifier
int dirCount=0;
for (const auto &dir : *Doxygen::dirLinkedMap)
{
dir->setDirCount(dirCount++);
}

computeCommonDirPrefix();
}
Expand Down
1 change: 1 addition & 0 deletions src/dirdef.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ class DirDef : public DefinitionMutable, public Definition
virtual void writeTagFile(TextStream &t) = 0;

virtual void setDiskName(const QCString &name) = 0;
virtual void setDirCount(int count) = 0;
virtual void sort() = 0;
virtual void setParent(DirDef *parent) = 0;
virtual void setLevel() = 0;
Expand Down

0 comments on commit 44e27fa

Please sign in to comment.