Skip to content

[clang][deps] Stop sorting file dependencies #114079

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

[clang][deps] Stop sorting file dependencies #114079

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
64c5459
[clang][deps] Ignore file dependency ordering in tests
jansvoboda11 Oct 22, 2024
19354f2
[clang][deps] Move file deps from a set into a vector, stop sorting
jansvoboda11 Oct 22, 2024
95b2328
[clang][deps] Add a TODO
jansvoboda11 Oct 29, 2024
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
2 changes: 1 addition & 1 deletion clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ struct ModuleDeps {

/// A collection of absolute paths to files that this module directly depends
/// on, not including transitive dependencies.
llvm::StringSet<> FileDeps;
std::vector<std::string> FileDeps;

/// A collection of absolute paths to module map files that this module needs
/// to know about. The ordering is significant.
Expand Down
10 changes: 5 additions & 5 deletions clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -779,23 +779,23 @@ static StringRef makeAbsoluteAndPreferred(CompilerInstance &CI, StringRef Path,
void ModuleDepCollector::addFileDep(StringRef Path) {
if (IsStdModuleP1689Format) {
// Within P1689 format, we don't want all the paths to be absolute path
// since it may violate the tranditional make style dependencies info.
FileDeps.push_back(std::string(Path));
// since it may violate the traditional make style dependencies info.
FileDeps.emplace_back(Path);
return;
}

llvm::SmallString<256> Storage;
Path = makeAbsoluteAndPreferred(ScanInstance, Path, Storage);
FileDeps.push_back(std::string(Path));
FileDeps.emplace_back(Path);
}

void ModuleDepCollector::addFileDep(ModuleDeps &MD, StringRef Path) {
if (IsStdModuleP1689Format) {
MD.FileDeps.insert(Path);
MD.FileDeps.emplace_back(Path);
return;
}

llvm::SmallString<256> Storage;
Path = makeAbsoluteAndPreferred(ScanInstance, Path, Storage);
MD.FileDeps.insert(Path);
MD.FileDeps.emplace_back(Path);
}
6 changes: 3 additions & 3 deletions clang/test/ClangScanDeps/diagnostics.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ module mod { header "mod.h" }
// CHECK: ],
// CHECK-NEXT: "context-hash": "[[HASH_MOD:.*]]",
// CHECK-NEXT: "file-deps": [
// CHECK-NEXT: "[[PREFIX]]/mod.h"
// CHECK-NEXT: "[[PREFIX]]/module.modulemap"
// CHECK-DAG: "[[PREFIX]]/mod.h"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm concerned that making all the tests use CHECK-DAG for dependencies will hide non-deterministic output in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We already have a test that diffs two scan results (submodule-order.c), so I believe that would catch non-deterministic ordering. (We might need to tweak the test so that one of the modules contains multiple files.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This test has very few files and isn't making any attempt to be exhaustive about which kinds of input files it has. I don't think it provides very good coverage for checking the order is deterministic.

// CHECK-DAG: "[[PREFIX]]/module.modulemap"
// CHECK-NEXT: ],
// CHECK-NEXT: "link-libraries": [],
// CHECK-NEXT: "name": "mod"
Expand All @@ -55,7 +55,7 @@ module mod { header "mod.h" }
// CHECK-NOT: "-fimplicit-module-maps"
// CHECK: ],
// CHECK: "file-deps": [
// CHECK-NEXT: "[[PREFIX]]/tu.c"
// CHECK-DAG: "[[PREFIX]]/tu.c"
// CHECK-NEXT: ],
// CHECK-NEXT: "input-file": "[[PREFIX]]/tu.c"
// CHECK-NEXT: }
30 changes: 15 additions & 15 deletions clang/test/ClangScanDeps/header-search-pruning-transitive.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,8 @@ module X { header "X.h" }
// CHECK: ],
// CHECK-NEXT: "context-hash": "[[HASH_X:.*]]",
// CHECK-NEXT: "file-deps": [
// CHECK-NEXT: "[[PREFIX]]/X.h",
// CHECK-NEXT: "[[PREFIX]]/module.modulemap"
// CHECK-DAG: "[[PREFIX]]/X.h"
// CHECK-DAG: "[[PREFIX]]/module.modulemap"
// CHECK-NEXT: ],
// CHECK-NEXT: "link-libraries": [],
// CHECK-NEXT: "name": "X"
Expand All @@ -85,11 +85,11 @@ module X { header "X.h" }
// CHECK: ],
// CHECK-NEXT: "context-hash": "[[HASH_Y_WITH_A]]",
// CHECK-NEXT: "file-deps": [
// CHECK-NEXT: "[[PREFIX]]/Y.h",
// CHECK-NEXT: "[[PREFIX]]/a/a.h",
// CHECK-NEXT: "[[PREFIX]]/begin/begin.h",
// CHECK-NEXT: "[[PREFIX]]/end/end.h",
// CHECK-NEXT: "[[PREFIX]]/module.modulemap"
// CHECK-DAG: "[[PREFIX]]/Y.h"
// CHECK-DAG: "[[PREFIX]]/a/a.h"
// CHECK-DAG: "[[PREFIX]]/begin/begin.h"
// CHECK-DAG: "[[PREFIX]]/end/end.h"
// CHECK-DAG: "[[PREFIX]]/module.modulemap"
// CHECK-NEXT: ],
// CHECK-NEXT: "link-libraries": [],
// CHECK-NEXT: "name": "Y"
Expand All @@ -107,7 +107,7 @@ module X { header "X.h" }
// CHECK-NEXT: "command-line": [
// CHECK: ],
// CHECK: "file-deps": [
// CHECK-NEXT: "[[PREFIX]]/test.c"
// CHECK-DAG: "[[PREFIX]]/test.c"
// CHECK-NEXT: ],
// CHECK-NEXT: "input-file": "[[PREFIX]]/test.c"
// CHECK-NEXT: }
Expand All @@ -128,8 +128,8 @@ module X { header "X.h" }
// also has a different context hash from the first version of module X.
// CHECK-NOT: "context-hash": "[[HASH_X]]",
// CHECK: "file-deps": [
// CHECK-NEXT: "[[PREFIX]]/X.h",
// CHECK-NEXT: "[[PREFIX]]/module.modulemap"
// CHECK-DAG: "[[PREFIX]]/X.h"
// CHECK-DAG: "[[PREFIX]]/module.modulemap"
// CHECK-NEXT: ],
// CHECK-NEXT: "link-libraries": [],
// CHECK-NEXT: "name": "X"
Expand All @@ -141,10 +141,10 @@ module X { header "X.h" }
// CHECK: ],
// CHECK-NEXT: "context-hash": "[[HASH_Y_WITHOUT_A]]",
// CHECK-NEXT: "file-deps": [
// CHECK-NEXT: "[[PREFIX]]/Y.h",
// CHECK-NEXT: "[[PREFIX]]/begin/begin.h",
// CHECK-NEXT: "[[PREFIX]]/end/end.h",
// CHECK-NEXT: "[[PREFIX]]/module.modulemap"
// CHECK-DAG: "[[PREFIX]]/Y.h"
// CHECK-DAG: "[[PREFIX]]/begin/begin.h"
// CHECK-DAG: "[[PREFIX]]/end/end.h"
// CHECK-DAG: "[[PREFIX]]/module.modulemap"
// CHECK-NEXT: ],
// CHECK-NEXT: "link-libraries": [],
// CHECK-NEXT: "name": "Y"
Expand All @@ -162,7 +162,7 @@ module X { header "X.h" }
// CHECK-NEXT: "command-line": [
// CHECK: ],
// CHECK: "file-deps": [
// CHECK-NEXT: "[[PREFIX]]/test.c"
// CHECK-DAG: "[[PREFIX]]/test.c"
// CHECK-NEXT: ],
// CHECK-NEXT: "input-file": "[[PREFIX]]/test.c"
// CHECK-NEXT: }
4 changes: 2 additions & 2 deletions clang/test/ClangScanDeps/line-directive.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@
// RUN: cat %t/deps.json | sed 's:\\\\\?:/:g' | FileCheck %s -DPREFIX=%/t

// CHECK: "file-deps": [
// CHECK-NEXT: "[[PREFIX]]/tu.c"
// CHECK-NEXT: "[[PREFIX]]/header.h"
// CHECK-DAG: "[[PREFIX]]/tu.c"
// CHECK-DAG: "[[PREFIX]]/header.h"
// CHECK-NEXT: ]

//--- cdb.json.template
Expand Down
20 changes: 10 additions & 10 deletions clang/test/ClangScanDeps/link-libraries.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,8 @@ module transitive {
// CHECK: ],
// CHECK-NEXT: "context-hash": "{{.*}}",
// CHECK-NEXT: "file-deps": [
// CHECK-NEXT: "[[PREFIX]]/Inputs/frameworks/Framework.framework/Headers/Framework.h"
// CHECK-NEXT: "[[PREFIX]]/Inputs/frameworks/module.modulemap"
// CHECK-DAG: "[[PREFIX]]/Inputs/frameworks/Framework.framework/Headers/Framework.h"
// CHECK-DAG: "[[PREFIX]]/Inputs/frameworks/module.modulemap"
// CHECK-NEXT: ],
// CHECK-NEXT: "link-libraries": [
// CHECK-NEXT: {
Expand All @@ -67,8 +67,8 @@ module transitive {
// CHECK: ],
// CHECK-NEXT: "context-hash": "{{.*}}",
// CHECK-NEXT: "file-deps": [
// CHECK-NEXT: "[[PREFIX]]/direct.h"
// CHECK-NEXT: "[[PREFIX]]/module.modulemap"
// CHECK-DAG: "[[PREFIX]]/direct.h"
// CHECK-DAG: "[[PREFIX]]/module.modulemap"
// CHECK-NEXT: ],
// CHECK-NEXT: "link-libraries": [],
// CHECK-NEXT: "name": "direct"
Expand All @@ -89,10 +89,10 @@ module transitive {
// CHECK: ],
// CHECK-NEXT: "context-hash": "{{.*}}",
// CHECK-NEXT: "file-deps": [
// CHECK-NEXT: "[[PREFIX]]/Inputs/frameworks/module.modulemap"
// CHECK-NEXT: "[[PREFIX]]/module.modulemap"
// CHECK-NEXT: "[[PREFIX]]/root.h"
// CHECK-NEXT: "[[PREFIX]]/root/textual.h"
// CHECK-DAG: "[[PREFIX]]/Inputs/frameworks/module.modulemap"
// CHECK-DAG: "[[PREFIX]]/module.modulemap"
// CHECK-DAG: "[[PREFIX]]/root.h"
// CHECK-DAG: "[[PREFIX]]/root/textual.h"
// CHECK-NEXT: ],
// CHECK-NEXT: "link-libraries": [],
// CHECK-NEXT: "name": "root"
Expand All @@ -104,8 +104,8 @@ module transitive {
// CHECK: ],
// CHECK-NEXT: "context-hash": "{{.*}}",
// CHECK-NEXT: "file-deps": [
// CHECK-NEXT: "[[PREFIX]]/module.modulemap"
// CHECK-NEXT: "[[PREFIX]]/transitive.h"
// CHECK-DAG: "[[PREFIX]]/module.modulemap"
// CHECK-DAG: "[[PREFIX]]/transitive.h"
// CHECK-NEXT: ],
// CHECK-NEXT: "link-libraries": [
// CHECK-NEXT: {
Expand Down
16 changes: 8 additions & 8 deletions clang/test/ClangScanDeps/modules-context-hash.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,9 @@
// CHECK: ],
// CHECK-NEXT: "context-hash": "[[HASH_MOD_A:.*]]",
// CHECK-NEXT: "file-deps": [
// CHECK-NEXT: "[[PREFIX]]/a/dep.h",
// CHECK-NEXT: "[[PREFIX]]/mod.h",
// CHECK-NEXT: "[[PREFIX]]/module.modulemap"
// CHECK-DAG: "[[PREFIX]]/a/dep.h"
// CHECK-DAG: "[[PREFIX]]/mod.h"
// CHECK-DAG: "[[PREFIX]]/module.modulemap"
// CHECK-NEXT: ],
// CHECK-NEXT: "link-libraries": [],
// CHECK-NEXT: "name": "mod"
Expand All @@ -54,7 +54,7 @@
// CHECK-NEXT: "command-line": [
// CHECK: ],
// CHECK: "file-deps": [
// CHECK-NEXT: "[[PREFIX]]/tu.c"
// CHECK-DAG: "[[PREFIX]]/tu.c"
// CHECK-NEXT: ],
// CHECK-NEXT: "input-file": "[[PREFIX]]/tu.c"
// CHECK-NEXT: }
Expand All @@ -72,9 +72,9 @@
// CHECK: ],
// CHECK-NOT: "context-hash": "[[HASH_MOD_A]]",
// CHECK: "file-deps": [
// CHECK-NEXT: "[[PREFIX]]/b/dep.h",
// CHECK-NEXT: "[[PREFIX]]/mod.h",
// CHECK-NEXT: "[[PREFIX]]/module.modulemap"
// CHECK-DAG: "[[PREFIX]]/b/dep.h"
// CHECK-DAG: "[[PREFIX]]/mod.h"
// CHECK-DAG: "[[PREFIX]]/module.modulemap"
// CHECK-NEXT: ],
// CHECK-NEXT: "link-libraries": [],
// CHECK-NEXT: "name": "mod"
Expand All @@ -92,7 +92,7 @@
// CHECK-NEXT: "command-line": [
// CHECK: ],
// CHECK: "file-deps": [
// CHECK-NEXT: "[[PREFIX]]/tu.c"
// CHECK-DAG: "[[PREFIX]]/tu.c"
// CHECK-NEXT: ],
// CHECK-NEXT: "input-file": "[[PREFIX]]/tu.c"
// CHECK-NEXT: }
Expand Down
10 changes: 5 additions & 5 deletions clang/test/ClangScanDeps/modules-dep-args.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,8 @@ module Direct { header "direct.h" }
// CHECK: ],
// CHECK-NEXT: "context-hash": "{{.*}}",
// CHECK-NEXT: "file-deps": [
// CHECK-NEXT: "[[PREFIX]]/direct.h",
// CHECK-NEXT: "[[PREFIX]]/module.modulemap"
// CHECK-DAG: "[[PREFIX]]/direct.h"
// CHECK-DAG: "[[PREFIX]]/module.modulemap"
// CHECK-NEXT: ],
// CHECK-NEXT: "link-libraries": [],
// CHECK-NEXT: "name": "Direct"
Expand All @@ -71,8 +71,8 @@ module Direct { header "direct.h" }
// CHECK: ],
// CHECK-NEXT: "context-hash": "{{.*}}",
// CHECK-NEXT: "file-deps": [
// CHECK-NEXT: "[[PREFIX]]/module.modulemap",
// CHECK-NEXT: "[[PREFIX]]/transitive.h"
// CHECK-DAG: "[[PREFIX]]/module.modulemap"
// CHECK-DAG: "[[PREFIX]]/transitive.h"
// CHECK-NEXT: ],
// CHECK-NEXT: "link-libraries": [],
// CHECK-NEXT: "name": "Transitive"
Expand All @@ -96,7 +96,7 @@ module Direct { header "direct.h" }
// CHECK_EAGER: "-fmodule-file=[[PREFIX]]/{{.*}}/Direct-{{.*}}.pcm"
// CHECK: ],
// CHECK: "file-deps": [
// CHECK-NEXT: "[[PREFIX]]/tu.c"
// CHECK-DAG: "[[PREFIX]]/tu.c"
// CHECK-NEXT: ],
// CHECK-NEXT: "input-file": "[[PREFIX]]/tu.c"
// CHECK-NEXT: }
20 changes: 10 additions & 10 deletions clang/test/ClangScanDeps/modules-extern-submodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,10 @@ module third {}
// CHECK: ],
// CHECK-NEXT: "context-hash": "{{.*}}",
// CHECK-NEXT: "file-deps": [
// CHECK-NEXT: "[[PREFIX]]/first/first/first.h",
// CHECK-NEXT: "[[PREFIX]]/first/first/module.modulemap",
// CHECK-NEXT: "[[PREFIX]]/second/second/module.modulemap",
// CHECK-NEXT: "[[PREFIX]]/second/second/sub.modulemap"
// CHECK-DAG: "[[PREFIX]]/first/first/first.h"
// CHECK-DAG: "[[PREFIX]]/first/first/module.modulemap"
// CHECK-DAG: "[[PREFIX]]/second/second/module.modulemap"
// CHECK-DAG: "[[PREFIX]]/second/second/sub.modulemap"
// CHECK-NEXT: ],
// CHECK-NEXT: "link-libraries": [],
// CHECK-NEXT: "name": "first"
Expand All @@ -72,10 +72,10 @@ module third {}
// CHECK: ],
// CHECK-NEXT: "context-hash": "{{.*}}",
// CHECK-NEXT: "file-deps": [
// CHECK-NEXT: "[[PREFIX]]/second/second/module.modulemap",
// CHECK-NEXT: "[[PREFIX]]/second/second/sub.h",
// CHECK-NEXT: "[[PREFIX]]/second/second/sub.modulemap",
// CHECK-NEXT: "[[PREFIX]]/third/module.modulemap"
// CHECK-DAG: "[[PREFIX]]/second/second/module.modulemap"
// CHECK-DAG: "[[PREFIX]]/second/second/sub.h"
// CHECK-DAG: "[[PREFIX]]/second/second/sub.modulemap"
// CHECK-DAG: "[[PREFIX]]/third/module.modulemap"
// CHECK-NEXT: ],
// CHECK-NEXT: "link-libraries": [],
// CHECK-NEXT: "name": "second"
Expand All @@ -90,7 +90,7 @@ module third {}
// CHECK: ],
// CHECK-NEXT: "context-hash": "{{.*}}",
// CHECK-NEXT: "file-deps": [
// CHECK-NEXT: "[[PREFIX]]/third/module.modulemap"
// CHECK-DAG: "[[PREFIX]]/third/module.modulemap"
// CHECK-NEXT: ],
// CHECK-NEXT: "link-libraries": [],
// CHECK-NEXT: "name": "third"
Expand All @@ -113,7 +113,7 @@ module third {}
// CHECK: "-fmodule-file=first=[[PREFIX]]/cache/{{.*}}/first-{{.*}}.pcm",
// CHECK: ],
// CHECK: "file-deps": [
// CHECK-NEXT: "[[PREFIX]]/tu.m"
// CHECK-DAG: "[[PREFIX]]/tu.m"
// CHECK-NEXT: ],
// CHECK-NEXT: "input-file": "[[PREFIX]]/tu.c"
// CHECK-NEXT: }
Expand Down
22 changes: 11 additions & 11 deletions clang/test/ClangScanDeps/modules-extern-unrelated.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
// CHECK: ],
// CHECK-NEXT: "context-hash": "{{.*}}",
// CHECK-NEXT: "file-deps": [
// CHECK-NEXT: "[[PREFIX]]/first/module.modulemap"
// CHECK-DAG: "[[PREFIX]]/first/module.modulemap"
// CHECK-NEXT: ],
// CHECK-NEXT: "link-libraries": [],
// CHECK-NEXT: "name": "first"
Expand All @@ -50,8 +50,8 @@
// CHECK: ],
// CHECK-NEXT: "context-hash": "{{.*}}",
// CHECK-NEXT: "file-deps": [
// CHECK-NEXT: "[[PREFIX]]/first/first_other.h",
// CHECK-NEXT: "[[PREFIX]]/first/module.modulemap"
// CHECK-DAG: "[[PREFIX]]/first/first_other.h"
// CHECK-DAG: "[[PREFIX]]/first/module.modulemap"
// CHECK-NEXT: ],
// CHECK-NEXT: "link-libraries": [],
// CHECK-NEXT: "name": "first_other"
Expand All @@ -69,9 +69,9 @@
// CHECK: ],
// CHECK-NEXT: "context-hash": "{{.*}}",
// CHECK-NEXT: "file-deps": [
// CHECK-NEXT: "[[PREFIX]]/first/module.modulemap",
// CHECK-NEXT: "[[PREFIX]]/second/second.h",
// CHECK-NEXT: "[[PREFIX]]/second/second.modulemap"
// CHECK-DAG: "[[PREFIX]]/first/module.modulemap"
// CHECK-DAG: "[[PREFIX]]/second/second.h"
// CHECK-DAG: "[[PREFIX]]/second/second.modulemap"
// CHECK-NEXT: ],
// CHECK-NEXT: "link-libraries": [],
// CHECK-NEXT: "name": "second"
Expand All @@ -93,10 +93,10 @@
// CHECK: ],
// CHECK-NEXT: "context-hash": "{{.*}}",
// CHECK-NEXT: "file-deps": [
// CHECK-NEXT: "[[PREFIX]]/first/module.modulemap",
// CHECK-NEXT: "[[PREFIX]]/second/second.modulemap",
// CHECK-NEXT: "[[PREFIX]]/zeroth/module.modulemap",
// CHECK-NEXT: "[[PREFIX]]/zeroth/zeroth.h"
// CHECK-DAG: "[[PREFIX]]/first/module.modulemap"
// CHECK-DAG: "[[PREFIX]]/second/second.modulemap"
// CHECK-DAG: "[[PREFIX]]/zeroth/module.modulemap"
// CHECK-DAG: "[[PREFIX]]/zeroth/zeroth.h"
// CHECK-NEXT: ],
// CHECK-NEXT: "link-libraries": [],
// CHECK-NEXT: "name": "zeroth"
Expand All @@ -117,7 +117,7 @@
// CHECK: ],
// CHECK-NEXT: "executable": "{{.*}}",
// CHECK-NEXT: "file-deps": [
// CHECK-NEXT: "[[PREFIX]]/tu.m"
// CHECK-DAG: "[[PREFIX]]/tu.m"
// CHECK-NEXT: ],
// CHECK-NEXT: "input-file": "[[PREFIX]]/tu.m"
// CHECK-NEXT: }
Expand Down
4 changes: 2 additions & 2 deletions clang/test/ClangScanDeps/modules-file-path-isolation.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@
// CHECK: "modules": [
// CHECK-NEXT: {
// CHECK: "file-deps": [
// CHECK-NEXT: "{{.*}}A.h",
// CHECK-NEXT: "{{.*}}module.modulemap"
// CHECK-DAG: "{{.*}}A.h"
// CHECK-DAG: "{{.*}}module.modulemap"
// CHECK-NEXT: ],
// CHECK-NEXT: "link-libraries": [],
// CHECK-NEXT: "name": "A"
Expand Down
Loading
Loading