[clang] [clang] Fix sorting header paths (PR #73323)
benlangmuir wrote:
You could try using `clang -cc1 -E -x c-module-map` which calls
`Module::print`. To trigger this code path you can follow the pattern in
`darwin_specific_modulemap_hacks.m`, ie create a module like
```
$ touch Tcl/x1.h
$ touch Tcl/x2.h
$ cat Tcl/module.modulemap
module Tcl {
module Private {
requires excluded
umbrella ""
}
}
$ clang -cc1 -E -x c-module-map Tcl/module.modulemap -fmodule-name=Tcl
# 1 "Tcl/module.modulemap"
module Tcl {
module Private {
textual header "" { size 0 mtime 1701196669 }
textual header "" { size 0 mtime 1701196741 }
textual header "" { size 75 mtime 1701196655 }
}
}
```
However I think the printing would need to be fixed to print the right header
name instead of "" first.
https://github.com/llvm/llvm-project/pull/73323
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Fix sorting header paths (PR #73323)
dwblaikie wrote: > > So what breakage is caused by the sorting failure? > > @dwblaikie This is not causing a breakage. It is just not working as designed > because the sort function has been comparing `""` against `""` since commit > [e6830b6](https://github.com/llvm/llvm-project/commit/e6830b6028ec5434ccf8dbebdd992918f67b1751) > . > > I found this while investigating issue #73145. > > The way the code is behaving now, the sort function is acting as a NOP and > could be removed. However, I don't think that was the intention of the author > of > [7ff2914](https://github.com/llvm/llvm-project/commit/7ff29148ac7883881e62dc9e1714057c68ad4436). > > > Can that behavior be tested in some way to validate this change and ensure > > it doesn't regress in the future? > > Possibly. Let me think on this. Yeah, looks like this was undertested when originally committed. Perhaps @benlangmuir can help sort out a test for this? Possibly creating a directory with a few headers & they'll appear in some arbitrary order from the OS in the `recursive_directory_iterator` - and then is there a way to print out the header map? We could print that out and demonstrate it is in the sorted order, not whatever order the iterator provided maybe. https://github.com/llvm/llvm-project/pull/73323 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Fix sorting header paths (PR #73323)
tuliom wrote: > So what breakage is caused by the sorting failure? @dwblaikie This is not causing a breakage. It is just not working as designed because the sort function has been comparing `""` against `""` since commit https://github.com/llvm/llvm-project/commit/e6830b6028ec5434ccf8dbebdd992918f67b1751 . I found this while investigating issue #73145. The way the code is behaving now, the sort function is acting as a NOP and could be removed. However, I don't think that was the intention of the author of https://github.com/llvm/llvm-project/commit/7ff29148ac7883881e62dc9e1714057c68ad4436. > Can that behavior be tested in some way to validate this change and ensure it > doesn't regress in the future? Possibly. Let me think on this. https://github.com/llvm/llvm-project/pull/73323 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Fix sorting header paths (PR #73323)
dwblaikie wrote: So what breakage is caused by the sorting failure? Can that behavior be tested in some way to validate this change and ensure it doesn't regress in the future? https://github.com/llvm/llvm-project/pull/73323 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Fix sorting header paths (PR #73323)
llvmbot wrote:
@llvm/pr-subscribers-clang-modules
@llvm/pr-subscribers-clang
Author: Tulio Magno Quites Machado Filho (tuliom)
Changes
This code was initially written in commit
7ff29148ac7883881e62dc9e1714057c68ad4436 with the intention of sorting headers
according to their path. At the time, the path was saved in field NameAsWritten
of Module::Header.
Later, commit e6830b6028ec5434ccf8dbebdd992918f67b1751 added field
PathRelativeToRootModuleDirectory to Module::Header and modified
ModuleMapParser::parseUmbrellaDirDecl() so that it started to save the header
path in the new field and started setting NameAsWritten = "". It didn't modify
compareModuleHeaders() in order to adapt it to the new field.
After this commit, the sorting stopped working because it continued comparing
only NameAsWritten.
This commit fixes it by treating NameAsWritten and
PathRelativeToRootModuleDirectory as a tuple when comparing Module::Header.
---
Full diff: https://github.com/llvm/llvm-project/pull/73323.diff
1 Files Affected:
- (modified) clang/lib/Lex/ModuleMap.cpp (+3-1)
``diff
diff --git a/clang/lib/Lex/ModuleMap.cpp b/clang/lib/Lex/ModuleMap.cpp
index 1d67e275cb4775a..7bc89b2fed36bf2 100644
--- a/clang/lib/Lex/ModuleMap.cpp
+++ b/clang/lib/Lex/ModuleMap.cpp
@@ -49,6 +49,7 @@
#include
#include
#include
+#include
#include
using namespace clang;
@@ -2511,7 +2512,8 @@ void ModuleMapParser::parseHeaderDecl(MMToken::TokenKind
LeadingToken,
static bool compareModuleHeaders(const Module::Header ,
const Module::Header ) {
- return A.NameAsWritten < B.NameAsWritten;
+ return std::tie(A.NameAsWritten, A.PathRelativeToRootModuleDirectory) <
+ std::tie(B.NameAsWritten, B.PathRelativeToRootModuleDirectory);
}
/// Parse an umbrella directory declaration.
``
https://github.com/llvm/llvm-project/pull/73323
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Fix sorting header paths (PR #73323)
https://github.com/tuliom created
https://github.com/llvm/llvm-project/pull/73323
This code was initially written in commit
7ff29148ac7883881e62dc9e1714057c68ad4436 with the intention of sorting headers
according to their path. At the time, the path was saved in field NameAsWritten
of Module::Header.
Later, commit e6830b6028ec5434ccf8dbebdd992918f67b1751 added field
PathRelativeToRootModuleDirectory to Module::Header and modified
ModuleMapParser::parseUmbrellaDirDecl() so that it started to save the header
path in the new field and started setting NameAsWritten = "". It didn't modify
compareModuleHeaders() in order to adapt it to the new field.
After this commit, the sorting stopped working because it continued comparing
only NameAsWritten.
This commit fixes it by treating NameAsWritten and
PathRelativeToRootModuleDirectory as a tuple when comparing Module::Header.
>From 772d51724dda39baa7347f2c33af5c65786964a2 Mon Sep 17 00:00:00 2001
From: Tulio Magno Quites Machado Filho
Date: Fri, 24 Nov 2023 09:26:12 -0300
Subject: [PATCH] [clang] Fix sorting header paths
This code was initially written in commit
7ff29148ac7883881e62dc9e1714057c68ad4436 with the intention of sorting
headers according to their path. At the time, the path was saved in
field NameAsWritten of Module::Header.
Later, commit e6830b6028ec5434ccf8dbebdd992918f67b1751 added field
PathRelativeToRootModuleDirectory to Module::Header and modified
ModuleMapParser::parseUmbrellaDirDecl() so that it started to save the
header path in the new field and started setting NameAsWritten = "".
It didn't modify compareModuleHeaders() in order to adapt it to the new
field.
After this commit, the sorting stopped working because it continued
comparing only NameAsWritten.
This commit fixes it by treating NameAsWritten and
PathRelativeToRootModuleDirectory as a tuple when comparing Module::Header.
---
clang/lib/Lex/ModuleMap.cpp | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/clang/lib/Lex/ModuleMap.cpp b/clang/lib/Lex/ModuleMap.cpp
index 1d67e275cb4775a..7bc89b2fed36bf2 100644
--- a/clang/lib/Lex/ModuleMap.cpp
+++ b/clang/lib/Lex/ModuleMap.cpp
@@ -49,6 +49,7 @@
#include
#include
#include
+#include
#include
using namespace clang;
@@ -2511,7 +2512,8 @@ void ModuleMapParser::parseHeaderDecl(MMToken::TokenKind
LeadingToken,
static bool compareModuleHeaders(const Module::Header ,
const Module::Header ) {
- return A.NameAsWritten < B.NameAsWritten;
+ return std::tie(A.NameAsWritten, A.PathRelativeToRootModuleDirectory) <
+ std::tie(B.NameAsWritten, B.PathRelativeToRootModuleDirectory);
}
/// Parse an umbrella directory declaration.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
