Skip to content

[clang-doc] fix flaky test in clang-doc #101387

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

Merged
merged 15 commits into from
Aug 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions clang-tools-extra/clang-doc/Representation.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -384,5 +384,12 @@ ClangDocContext::ClangDocContext(tooling::ExecutionContext *ECtx,
}
}

void ScopeChildren::sort() {
llvm::sort(Namespaces.begin(), Namespaces.end());
llvm::sort(Records.begin(), Records.end());
llvm::sort(Functions.begin(), Functions.end());
llvm::sort(Enums.begin(), Enums.end());
llvm::sort(Typedefs.begin(), Typedefs.end());
}
} // namespace doc
} // namespace clang
28 changes: 26 additions & 2 deletions clang-tools-extra/clang-doc/Representation.h
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ struct Reference {

bool mergeable(const Reference &Other);
void merge(Reference &&I);
bool operator<(const Reference &Other) const { return Name < Other.Name; }

/// Returns the path for this Reference relative to CurrentPath.
llvm::SmallString<64> getRelativeFilePath(const StringRef &CurrentPath) const;
Expand Down Expand Up @@ -145,6 +146,8 @@ struct ScopeChildren {
std::vector<FunctionInfo> Functions;
std::vector<EnumInfo> Enums;
std::vector<TypedefInfo> Typedefs;

void sort();
};

// A base struct for TypeInfos
Expand Down Expand Up @@ -245,6 +248,11 @@ struct Location {
std::tie(Other.LineNumber, Other.Filename);
}

bool operator!=(const Location &Other) const {
return std::tie(LineNumber, Filename) !=
std::tie(Other.LineNumber, Other.Filename);
}

// This operator is used to sort a vector of Locations.
// No specific order (attributes more important than others) is required. Any
// sort is enough, the order is only needed to call std::unique after sorting
Expand All @@ -270,10 +278,12 @@ struct Info {

virtual ~Info() = default;

Info &operator=(Info &&Other) = default;

SymbolID USR =
SymbolID(); // Unique identifier for the decl described by this Info.
const InfoType IT = InfoType::IT_default; // InfoType of this particular Info.
SmallString<16> Name; // Unqualified name of the decl.
InfoType IT = InfoType::IT_default; // InfoType of this particular Info.
SmallString<16> Name; // Unqualified name of the decl.
llvm::SmallVector<Reference, 4>
Namespace; // List of parent namespaces for this decl.
std::vector<CommentInfo> Description; // Comment description of this decl.
Expand Down Expand Up @@ -312,6 +322,20 @@ struct SymbolInfo : public Info {

std::optional<Location> DefLoc; // Location where this decl is defined.
llvm::SmallVector<Location, 2> Loc; // Locations where this decl is declared.

bool operator<(const SymbolInfo &Other) const {
// Sort by declaration location since we want the doc to be
// generated in the order of the source code.
Comment on lines +327 to +328
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation often sorts methods by name, so its easy to find things. As an example, here's Rust's Vec documentation https://doc.rust-lang.org/std/vec/struct.Vec.html. Methods are sorted in the navigation window and within the body, despite not being that way in source.

Doxygen maintains source order, so its fine to do it that way for now, but we may want to consider making that a configurable option, since I think its more user friendly to do it that way (and arguably more modern).

// If the declaration location is the same, or not present
// we sort by defined location otherwise fallback to the extracted name
if (Loc.size() > 0 && Other.Loc.size() > 0 && Loc[0] != Other.Loc[0])
return Loc[0] < Other.Loc[0];

if (DefLoc && Other.DefLoc && *DefLoc != *Other.DefLoc)
return *DefLoc < *Other.DefLoc;

return extractName() < Other.extractName();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the overall comparison, you could probably use std::tuple to make the logic simpler. The optional value may make that harder though, as I don't recall offhand how that's normally handled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not too familiar with what you are talking about can you give an example?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}
};

// TODO: Expand to allow for documenting templating and default args.
Expand Down
18 changes: 18 additions & 0 deletions clang-tools-extra/clang-doc/tool/ClangDocMain.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,22 @@ llvm::Error getHtmlAssetFiles(const char *Argv0,
return getDefaultAssetFiles(Argv0, CDCtx);
}

/// Make the output of clang-doc deterministic by sorting the children of
/// namespaces and records.
void sortUsrToInfo(llvm::StringMap<std::unique_ptr<doc::Info>> &USRToInfo) {
for (auto &I : USRToInfo) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does USRToInfo itself need to be in a sorted/stable order?

Copy link
Contributor Author

@PeterChou1 PeterChou1 Aug 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No since USRToInfo is generated on a per file basis so there's no need to sort it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't necessarily rule out its own iteration order affecting the output.

Copy link
Contributor Author

@PeterChou1 PeterChou1 Aug 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How so? StringMap iteration order is not guaranteed to be deterministic and I don't think it was affecting the tests before

auto &Info = I.second;
if (Info->IT == doc::InfoType::IT_namespace) {
auto *Namespace = static_cast<clang::doc::NamespaceInfo *>(Info.get());
Namespace->Children.sort();
}
if (Info->IT == doc::InfoType::IT_record) {
auto *Record = static_cast<clang::doc::RecordInfo *>(Info.get());
Record->Children.sort();
}
}
}

int main(int argc, const char **argv) {
llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
std::error_code OK;
Expand Down Expand Up @@ -341,6 +357,8 @@ Example usage for a project using a compile commands database:
if (Error)
return 1;

sortUsrToInfo(USRToInfo);

// Ensure the root output directory exists.
if (std::error_code Err = llvm::sys::fs::create_directories(OutDirectory);
Err != std::error_code()) {
Expand Down
9 changes: 3 additions & 6 deletions clang-tools-extra/test/clang-doc/basic-project.test
Original file line number Diff line number Diff line change
@@ -1,6 +1,3 @@
// See https://github.com/llvm/llvm-project/issues/97507.
// UNSUPPORTED: target={{.*}}

// RUN: rm -rf %t && mkdir -p %t/docs %t/build
// RUN: sed 's|$test_dir|%/S|g' %S/Inputs/basic-project/database_template.json > %t/build/compile_commands.json
// RUN: clang-doc --format=html --output=%t/docs --executor=all-TUs %t/build/compile_commands.json
Expand Down Expand Up @@ -61,13 +58,13 @@
// HTML-SHAPE: <p>Defined at line 8 of file {{.*}}Shape.h</p>
// HTML-SHAPE: <p> Provides a common interface for different types of shapes.</p>
// HTML-SHAPE: <h2 id="Functions">Functions</h2>
// HTML-SHAPE: <h3 id="{{([0-9A-F]{40})}}">~Shape</h3>
// HTML-SHAPE: <p>public void ~Shape()</p>
// HTML-SHAPE: <p>Defined at line 13 of file {{.*}}Shape.h</p>
// HTML-SHAPE: <h3 id="{{([0-9A-F]{40})}}">area</h3>
// HTML-SHAPE: <p>public double area()</p>
// HTML-SHAPE: <h3 id="{{([0-9A-F]{40})}}">perimeter</h3>
// HTML-SHAPE: <p>public double perimeter()</p>
// HTML-SHAPE: <h3 id="{{([0-9A-F]{40})}}">~Shape</h3>
// HTML-SHAPE: <p>public void ~Shape()</p>
// HTML-SHAPE: <p>Defined at line 13 of file {{.*}}Shape.h</p>

// HTML-CALC: <h1>class Calculator</h1>
// HTML-CALC: <p>Defined at line 8 of file {{.*}}Calculator.h</p>
Expand Down
6 changes: 4 additions & 2 deletions clang-tools-extra/test/clang-doc/namespace.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -272,14 +272,16 @@ namespace AnotherNamespace {
// HTML-GLOBAL-INDEX: <h1>Global Namespace</h1>
// HTML-GLOBAL-INDEX: <h2 id="Namespaces">Namespaces</h2>
// HTML-GLOBAL-INDEX: <li>@nonymous_namespace</li>
// HTML-GLOBAL-INDEX: <li>PrimaryNamespace</li>
// HTML-GLOBAL-INDEX: <li>AnotherNamespace</li>
// HTML-GLOBAL-INDEX: <li>PrimaryNamespace</li>


// MD-GLOBAL-INDEX: # Global Namespace
// MD-GLOBAL-INDEX: ## Namespaces
// MD-GLOBAL-INDEX: * [@nonymous_namespace](..{{[\/]}}@nonymous_namespace{{[\/]}}index.md)
// MD-GLOBAL-INDEX: * [PrimaryNamespace](..{{[\/]}}PrimaryNamespace{{[\/]}}index.md)
// MD-GLOBAL-INDEX: * [AnotherNamespace](..{{[\/]}}AnotherNamespace{{[\/]}}index.md)
// MD-GLOBAL-INDEX: * [PrimaryNamespace](..{{[\/]}}PrimaryNamespace{{[\/]}}index.md)


// MD-ALL-FILES: # All Files
// MD-ALL-FILES: ## [@nonymous_namespace](@nonymous_namespace{{[\/]}}index.md)
Expand Down
34 changes: 17 additions & 17 deletions clang-tools-extra/test/clang-doc/templates.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,23 @@ void ParamPackFunction(T... args);
// CHECK: ---
// CHECK-NEXT: USR: '{{([0-9A-F]{40})}}'
// CHECK-NEXT: ChildFunctions:
// CHECK-NEXT: - USR: '{{([0-9A-F]{40})}}'
// CHECK-NEXT: Name: 'ParamPackFunction'
// CHECK-NEXT: Location:
// CHECK-NEXT: - LineNumber: 16
// CHECK-NEXT: Filename: '{{.*}}'
// CHECK-NEXT: Params:
// CHECK-NEXT: - Type:
// CHECK-NEXT: Name: 'T...'
// CHECK-NEXT: QualName: 'T...'
// CHECK-NEXT: Name: 'args'
// CHECK-NEXT: ReturnType:
// CHECK-NEXT: Type:
// CHECK-NEXT: Name: 'void'
// CHECK-NEXT: QualName: 'void'
// CHECK-NEXT: Template:
// CHECK-NEXT: Params:
// CHECK-NEXT: - Contents: 'class... T'
// CHECK-NEXT: - USR: '{{([0-9A-F]{40})}}'
// CHECK-NEXT: Name: 'function'
// CHECK-NEXT: DefLocation:
Expand Down Expand Up @@ -56,21 +73,4 @@ void ParamPackFunction(T... args);
// CHECK-NEXT: Params:
// CHECK-NEXT: - Contents: 'bool'
// CHECK-NEXT: - Contents: '0'
// CHECK-NEXT: - USR: '{{([0-9A-F]{40})}}'
// CHECK-NEXT: Name: 'ParamPackFunction'
// CHECK-NEXT: Location:
// CHECK-NEXT: - LineNumber: 16
// CHECK-NEXT: Filename: '{{.*}}'
// CHECK-NEXT: Params:
// CHECK-NEXT: - Type:
// CHECK-NEXT: Name: 'T...'
// CHECK-NEXT: QualName: 'T...'
// CHECK-NEXT: Name: 'args'
// CHECK-NEXT: ReturnType:
// CHECK-NEXT: Type:
// CHECK-NEXT: Name: 'void'
// CHECK-NEXT: QualName: 'void'
// CHECK-NEXT: Template:
// CHECK-NEXT: Params:
// CHECK-NEXT: - Contents: 'class... T'
// CHECK-NEXT: ...