https://github.com/jansvoboda11 closed
https://github.com/llvm/llvm-project/pull/89441
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/jansvoboda11 edited
https://github.com/llvm/llvm-project/pull/89441
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/jansvoboda11 updated
https://github.com/llvm/llvm-project/pull/89441
>From 9165d6086e2570198fba1c333d0c9cb9c09037c7 Mon Sep 17 00:00:00 2001
From: Jan Svoboda
Date: Fri, 19 Apr 2024 12:13:06 -0700
Subject: [PATCH 1/8] [clang][modules] Allow module map files with textual
@@ -1574,6 +1574,7 @@ bool HeaderSearch::ShouldEnterIncludeFile(Preprocessor
,
}
}
+ FileInfo.IsLocallyIncluded = true;
jansvoboda11 wrote:
I agree, I wanted to colocate this with call to `Preprocessor::markIncluded()`.
If we find a better way of
https://github.com/sam-mccall edited
https://github.com/llvm/llvm-project/pull/89441
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/sam-mccall approved this pull request.
Thanks, LGTM
https://github.com/llvm/llvm-project/pull/89441
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -1574,6 +1574,7 @@ bool HeaderSearch::ShouldEnterIncludeFile(Preprocessor
,
}
}
+ FileInfo.IsLocallyIncluded = true;
sam-mccall wrote:
I'd consider placing this at the end of HandleHeaderIncludeOrImport rather than
here:
- it looks like there
@@ -2057,9 +2065,12 @@ void ASTWriter::WriteHeaderSearch(const HeaderSearch
) {
// it as a header file (in which case HFI will be null) or if it hasn't
// changed since it was loaded. Also skip it if it's for a modular header
// from a different module; in that
@@ -0,0 +1,20 @@
+// This test checks that a module map with a textual header can be marked as
jansvoboda11 wrote:
(And thanks for the extended test case!)
https://github.com/llvm/llvm-project/pull/89441
___
@@ -0,0 +1,20 @@
+// This test checks that a module map with a textual header can be marked as
jansvoboda11 wrote:
Should work with
[b03c34f](https://github.com/llvm/llvm-project/pull/89441/commits/b03c34f99bd90398f0599b5703cbf82d8b881981).
https://github.com/jansvoboda11 updated
https://github.com/llvm/llvm-project/pull/89441
>From 9165d6086e2570198fba1c333d0c9cb9c09037c7 Mon Sep 17 00:00:00 2001
From: Jan Svoboda
Date: Fri, 19 Apr 2024 12:13:06 -0700
Subject: [PATCH 1/6] [clang][modules] Allow module map files with textual
https://github.com/jansvoboda11 updated
https://github.com/llvm/llvm-project/pull/89441
>From 9165d6086e2570198fba1c333d0c9cb9c09037c7 Mon Sep 17 00:00:00 2001
From: Jan Svoboda
Date: Fri, 19 Apr 2024 12:13:06 -0700
Subject: [PATCH 1/5] [clang][modules] Allow module map files with textual
github-actions[bot] wrote:
:warning: C/C++ code formatter, clang-format found issues in your code.
:warning:
You can test this locally with the following command:
``bash
git-clang-format --diff 3ea5dff0efdbb4bf5590d882810aa42f9ec26e4e
ac069f486a197221adb6520d76ac8aaa74995947 --
https://github.com/jansvoboda11 updated
https://github.com/llvm/llvm-project/pull/89441
>From 9165d6086e2570198fba1c333d0c9cb9c09037c7 Mon Sep 17 00:00:00 2001
From: Jan Svoboda
Date: Fri, 19 Apr 2024 12:13:06 -0700
Subject: [PATCH 1/5] [clang][modules] Allow module map files with textual
@@ -0,0 +1,20 @@
+// This test checks that a module map with a textual header can be marked as
jansvoboda11 wrote:
This patch unfortunately doesn't pass that test (on line 44) due to
[this](https://github.com/llvm/llvm-project/pull/89441/files#r1576422103):
https://github.com/jansvoboda11 updated
https://github.com/llvm/llvm-project/pull/89441
>From 9165d6086e2570198fba1c333d0c9cb9c09037c7 Mon Sep 17 00:00:00 2001
From: Jan Svoboda
Date: Fri, 19 Apr 2024 12:13:06 -0700
Subject: [PATCH 1/4] [clang][modules] Allow module map files with textual
@@ -237,6 +238,8 @@ GetAffectingModuleMaps(const Preprocessor , Module
*RootModule) {
CollectIncludingMapsFromAncestors(CurrentModule);
for (const Module *ImportedModule : CurrentModule->Imports)
CollectIncludingMapsFromAncestors(ImportedModule);
+for (const
@@ -187,7 +187,8 @@ GetAffectingModuleMaps(const Preprocessor , Module
*RootModule) {
continue;
const HeaderFileInfo *HFI = HS.getExistingLocalFileInfo(*File);
-if (!HFI || (HFI->isModuleHeader && !HFI->isCompilingModuleHeader))
+if (!HFI ||
@@ -0,0 +1,20 @@
+// This test checks that a module map with a textual header can be marked as
sam-mccall wrote:
This is a useful test, I think there are a couple of other affecting-ness
properties that are important to test:
- that a textual header that *is*
https://github.com/sam-mccall approved this pull request.
Thanks! This looks good, much neater than my approach.
I'm interested in whether the DirectUses change is related to this, and would
like to slap on a couple of tests.
But either way, this looks good and it would be great to have it
https://github.com/sam-mccall edited
https://github.com/llvm/llvm-project/pull/89441
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -187,7 +187,8 @@ GetAffectingModuleMaps(const Preprocessor , Module
*RootModule) {
continue;
const HeaderFileInfo *HFI = HS.getExistingLocalFileInfo(*File);
-if (!HFI || (HFI->isModuleHeader && !HFI->isCompilingModuleHeader))
+if (!HFI ||
jansvoboda11 wrote:
> I've sent a version of this as #89729, based on your first commit here.
I left a comment there.
> > I think one option we have here is to consider all module maps describing a
> > textual header that got included as affecting. I'm concerned that a long
> > chain of
https://github.com/jansvoboda11 updated
https://github.com/llvm/llvm-project/pull/89441
>From 9165d6086e2570198fba1c333d0c9cb9c09037c7 Mon Sep 17 00:00:00 2001
From: Jan Svoboda
Date: Fri, 19 Apr 2024 12:13:06 -0700
Subject: [PATCH 1/3] [clang][modules] Allow module map files with textual
sam-mccall wrote:
> @sam-mccall That makes sense.
>
> I think one option we have here is to consider all module maps describing a
> textual header that got included as affecting. I'm concerned that a long
> chain of textual header includes might again be problematic.
Yeah, that's the option
jansvoboda11 wrote:
@sam-mccall That makes sense.
I think one option we have here is to consider all module maps describing a
textual header that got included as affecting. I'm concerned that a long chain
of textual header includes might again be problematic.
For this particular test, we
https://github.com/jansvoboda11 updated
https://github.com/llvm/llvm-project/pull/89441
>From 9165d6086e2570198fba1c333d0c9cb9c09037c7 Mon Sep 17 00:00:00 2001
From: Jan Svoboda
Date: Fri, 19 Apr 2024 12:13:06 -0700
Subject: [PATCH 1/2] [clang][modules] Allow module map files with textual
sam-mccall wrote:
> I updated the description of this PR, hopefully it makes more sense now. I
> still need to investigate what goes wrong in
> "Modules/preprocess-decluse.cpp". It seems that it assumes `%t/b.pcm` embeds
> the information from "a.modulemap".
I think it should embed that
ian-twilightcoder wrote:
> > Is this a pre-existing issue, or did my patch change to make "each textual
> > header gets a `HFI`"?
>
> My best understanding that your patch gave textual headers`HFI`s when the
> module map was loaded, rather than when the header was included. This
> shouldn't
sam-mccall wrote:
> Is this a pre-existing issue, or did my patch change to make "each textual
> header gets a `HFI`"?
My best understanding that your patch gave textual headers`HFI`s when the
module map was loaded, rather than when the header was included. This shouldn't
have mattered, but
sam-mccall wrote:
> clang's headers all have proper modules now, are you sure you still need A?
We can't use `clang/lib/Headers/module.modulemap`, so we need something to
describe those headers.
Why can't we? In our build system, it's the **build system's** job to generate
modulemaps for
jansvoboda11 wrote:
> Is this a pre-existing issue, or did my patch change to make "each textual
> header gets a `HFI`"?
I'm not sure yet, I think we'll need to investigate [Sam's
reproducer](https://github.com/llvm/llvm-project/pull/89005#issuecomment-2067300965)
and consider [Richard's
ian-twilightcoder wrote:
> I updated the description of this PR, hopefully it makes more sense now. I
> still need to investigate what goes wrong in
> "Modules/preprocess-decluse.cpp". It seems that it assumes `%t/b.pcm` embeds
> the information from "a.modulemap".
Is this a pre-existing
jansvoboda11 wrote:
I updated the description of this PR, hopefully it makes more sense now. I
still need to investigate what goes wrong in "Modules/preprocess-decluse.cpp".
It seems that it assumes `%t/b.pcm` embeds the information from "a.modulemap".
https://github.com/jansvoboda11 edited
https://github.com/llvm/llvm-project/pull/89441
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
ian-twilightcoder wrote:
> > Hmm, this will probably only work if you compile `A` into a PCM and then
> > pass it to `B`. This is not how "Modules/preprocess-decluse.cpp" is set up.
>
> In our case we always have a chain A <- B <- C. A.modulemap can be affecting
> for B but should not be for
https://github.com/ian-twilightcoder approved this pull request.
I don't totally understand the change, but I don't see anything wrong with it!
https://github.com/llvm/llvm-project/pull/89441
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://github.com/sam-mccall approved this pull request.
This makes sense to me. It corresponds to our build structure and fixes the new
build failures we saw after 0cd0aa029647c8d1dba5c3d62f92325576796fa2.
Really appreciate your work on this!
https://github.com/llvm/llvm-project/pull/89441
sam-mccall wrote:
> Hmm, this will probably only work if you compile `A` into a PCM and then pass
> it to `B`. This is not how "Modules/preprocess-decluse.cpp" is set up.
In our case we always have a chain A <- B <- C.
A.modulemap can be affecting for B but should not be for C.
(Approximately,
sam-mccall wrote:
I can confirm applying this allows our targets to build again! :tada:
Thank you, will take a look at the implementation now.
https://github.com/llvm/llvm-project/pull/89441
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
jansvoboda11 wrote:
Hmm, this will probably only work if you compile `A` into a PCM and then pass
it to `B`. This is not how "Modules/preprocess-decluse.cpp" is set up.
https://github.com/llvm/llvm-project/pull/89441
___
cfe-commits mailing list
https://github.com/jansvoboda11 edited
https://github.com/llvm/llvm-project/pull/89441
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/jansvoboda11 updated
https://github.com/llvm/llvm-project/pull/89441
>From 9165d6086e2570198fba1c333d0c9cb9c09037c7 Mon Sep 17 00:00:00 2001
From: Jan Svoboda
Date: Fri, 19 Apr 2024 12:13:06 -0700
Subject: [PATCH] [clang][modules] Allow module map files with textual header
https://github.com/jansvoboda11 edited
https://github.com/llvm/llvm-project/pull/89441
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
44 matches
Mail list logo
