@@ -1403,94 +1421,146 @@ bool
HeaderSearch::isFileMultipleIncludeGuarded(FileEntryRef File) const {
void HeaderSearch::MarkFileModuleHeader(FileEntryRef FE,
ModuleMap::ModuleHeaderRole Role,
bool
@@ -1403,94 +1421,146 @@ bool
HeaderSearch::isFileMultipleIncludeGuarded(FileEntryRef File) const {
void HeaderSearch::MarkFileModuleHeader(FileEntryRef FE,
ModuleMap::ModuleHeaderRole Role,
bool
@@ -1403,94 +1421,146 @@ bool
HeaderSearch::isFileMultipleIncludeGuarded(FileEntryRef File) const {
void HeaderSearch::MarkFileModuleHeader(FileEntryRef FE,
ModuleMap::ModuleHeaderRole Role,
bool
ian-twilightcoder wrote:
> To get back to my previous point about the semantics implemented by this
> patch being unfortunate -- the upshot is that, now:
>
> ```
> #include
> #define NDEBUG
> #import
> ```
It would be nice if we could make this work without modules too. `#pragma many`
or
ian-twilightcoder wrote:
Wall of text incoming... Sorry but our documentation in this area is poor, so I
feel like I need to spell out the assumptions behind this change.
First of all, this _should_ be a relatively minor change that should affect a
very limited set of circumstances.
1. Only
jyknight wrote:
To get back to my previous point about the semantics implemented by this patch
being unfortunate -- the upshot is that, now:
```
#include
#define NDEBUG
#import
```
will include assert.h twice (even though the latter is an "import") _only_ if
modules are enabled. If modules
jyknight wrote:
>> The end result should be that #imported and #pragma once-guarded files are
>> treated the same way as #ifndef-guarded files.
> While I don't necessarily disagree with that goal in principle, it wasn't
> true before this change either.
Well, yes, I know it isn't true yet --
ian-twilightcoder wrote:
> The end result should be that #imported and #pragma once-guarded files are
> treated the same way as #ifndef-guarded files.
While I don't necessarily disagree with that goal in principle, it wasn't true
before this change either. There was already some special
jyknight wrote:
> The problem I'm trying to fix is that nobody knows when it's appropriate to
> use #import vs #include
But you haven't really (and I think cannot) fixed that.
> using header guards or #pragma once is very "un-Objective-C".
Yes, this is quite unfortunate. The best answer
ian-twilightcoder wrote:
Also this change isn't trying to modify `#include` in any kind of way, _only_
`#import`. So it's quite intentional that your test behaves the same before and
after.
https://github.com/llvm/llvm-project/pull/83660
___
ian-twilightcoder wrote:
I don't really think it's the same thing. The problem I'm trying to fix is that
nobody knows when it's appropriate to use `#import` vs `#include`, and the
unfortunate convention of Objective-C makes it impossible for header owners to
indicate if they support being
jyknight wrote:
Oh -- I'd also note that even if this is reverted, ilya-biryukov may want to
continue to investigate the source-location issue -- it's entirely possible
that the correct fix will trigger that same problem!
https://github.com/llvm/llvm-project/pull/83660
jyknight wrote:
I think the bug this change was attempting to fix is actually the same as
#38554 and related bugs -- which, it appears, was not really fixed.
The underlying problem here is that `#pragma once` should should work
identically to ifndef guards, as far as what macros/decls are
ian-twilightcoder wrote:
Wow that's a _ton_ of PCM files. Why are your headers `textual` if they're only
meant to be included once? Do they need to inherit the includer's preprocessor
environment or something like that?
https://github.com/llvm/llvm-project/pull/83660
ilya-biryukov wrote:
I still don't understand why the number of `FileID`s for module maps has
increased so much. I am observing this increase when compiling a file that has
many module dependencies, it creates only one `FileID` for a module map I
mentioned above, the rest 2259 are from loaded
ilya-biryukov wrote:
> Are your `textual` headers meant to be included more than once? If not, do
> they use header guards or `#pragma once`?
As I mentioned, the increase is coming from the module files.
The textual headers are header-guarded and not intended to be included more
than once.
>
ian-twilightcoder wrote:
> It's definitely caused by that change, reverting it fixes the failure.
>
> We use module maps so that some of our `#include` get processed as `#import`,
> I believe the same code gets executed for our use-case. Because our setup is
> so unusual, producing a repro is
ian-twilightcoder wrote:
Are your `textual` headers meant to be included more than once? If not, do they
use header guards or `#pragma once`?
https://github.com/llvm/llvm-project/pull/83660
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
ilya-biryukov wrote:
Most of the increase seems to be coming from module maps that describe textual
headers:
```
// This describes builtin headers.
Before: gen-crosstool-x86.cppmap:1:1: note: file entered 630 times using
83989080B of space
After: gen-crosstool-x86.cppmap:1:1: note: file
ilya-biryukov wrote:
It's definitely caused by that change, reverting it fixes the failure.
We use module maps so that some of our `#include` get processed as `#import`, I
believe the same code gets executed for our use-case. Because our setup is so
unusual, producing a repro is hard and make
ian-twilightcoder wrote:
> @ian-twilightcoder this change seemed to cause our internal builds to run out
> of source location space.
>
> ```
> remark: source manager location address space usage: [-Rsloc-usage]
> note: 408559B in local locations, 2146921126B in locations loaded from AST
>
ilya-biryukov wrote:
@ian-twilightcoder this change seemed to cause our internal builds to run out
of source location space.
```
remark: source manager location address space usage: [-Rsloc-usage]
note: 408559B in local locations, 2146921126B in locations loaded from AST
files, for a total of
https://github.com/ian-twilightcoder closed
https://github.com/llvm/llvm-project/pull/83660
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/jansvoboda11 approved this pull request.
LGTM
https://github.com/llvm/llvm-project/pull/83660
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/ian-twilightcoder updated
https://github.com/llvm/llvm-project/pull/83660
>From 63ff00ec49ac20c5ac97bd673166dabb0fb56136 Mon Sep 17 00:00:00 2001
From: Ian Anderson
Date: Fri, 1 Mar 2024 22:17:09 -0800
Subject: [PATCH] [clang][modules] Headers meant to be included multiple
ian-twilightcoder wrote:
> > Sometimes it does confuse clang, at least I saw problems with a `typedef
> > enum` when I made an include-once header `textual`.
>
> Ok, I see. I would just consider it a bug, not a design decision.
>
> > That's correct. `#import` is an external source - often it
vsapsai wrote:
> Sometimes it does confuse clang, at least I saw problems with a `typedef
> enum` when I made an include-once header `textual`.
Ok, I see. I would just consider it a bug, not a design decision.
> That's correct. `#import` is an external source - often it comes from the
>
ian-twilightcoder wrote:
> To clarify a little bit
>
> > [...] The "already included" state is global across all modules (which is
> > necessary so that non-modular headers don't get compiled into multiple
> > translation units and cause redeclaration errors).
>
> The necessity isn't
vsapsai wrote:
To clarify a little bit
> [...] The "already included" state is global across all modules (which is
> necessary so that non-modular headers don't get compiled into multiple
> translation units and cause redeclaration errors).
The necessity isn't actually true. The same
https://github.com/ian-twilightcoder updated
https://github.com/llvm/llvm-project/pull/83660
>From 1cb3d459f3a9ae73ac98bf8c06b905d788be954f Mon Sep 17 00:00:00 2001
From: Ian Anderson
Date: Fri, 1 Mar 2024 22:17:09 -0800
Subject: [PATCH] [clang][modules] Headers meant to be included multiple
ian-twilightcoder wrote:
> Thanks to @Bigcheese for helping with this patch.
>
> https://reviews.llvm.org/D26267 looks like the most recent significant update
> to this code that we could find. It had an additional allowance where clang
> headers that were _not_ marked `textual` could be
https://github.com/ian-twilightcoder updated
https://github.com/llvm/llvm-project/pull/83660
>From f90fe8b1b7c73b68614ade3cf7e1c292f02d3573 Mon Sep 17 00:00:00 2001
From: Ian Anderson
Date: Fri, 1 Mar 2024 22:17:09 -0800
Subject: [PATCH] [clang][modules] Headers meant to be included multiple
https://github.com/ian-twilightcoder updated
https://github.com/llvm/llvm-project/pull/83660
>From 08681ff77f432806316109146ab4fef058137648 Mon Sep 17 00:00:00 2001
From: Ian Anderson
Date: Fri, 1 Mar 2024 22:17:09 -0800
Subject: [PATCH] [clang][modules] Headers meant to be included multiple
ian-twilightcoder wrote:
> > Once a file has been `#import`'ed, it gets stamped as if it was `#pragma
> > once` and will not be re-entered, even on #include.
>
> Can you explain how this is happening? The only place where
> `HeaderFileInfo::isPragmaOnce` is set to `true` is in
>
jansvoboda11 wrote:
> Once a file has been `#import`'ed, it gets stamped as if it was `#pragma
> once` and will not be re-entered, even on #include.
Can you explain how this is happening? The only place where
`HeaderFileInfo::isPragmaOnce` is set to `true` is in
ian-twilightcoder wrote:
```
error: 'expected-error' diagnostics seen but not expected:
File
/var/lib/buildkite-agent/builds/linux-56-59b8f5d88-6mhbj-1/llvm-project/clang-ci/clang/test/Modules/explicit-build-overlap.cpp
Line 11: module use does not directly depend on a module exporting
https://github.com/ian-twilightcoder updated
https://github.com/llvm/llvm-project/pull/83660
>From 771c0740dcc438d99baf4ad9555bc57e963001df Mon Sep 17 00:00:00 2001
From: Ian Anderson
Date: Fri, 1 Mar 2024 22:17:09 -0800
Subject: [PATCH] [clang][modules] Headers meant to be included multiple
ian-twilightcoder wrote:
Thanks to @Bigcheese for helping with this patch.
https://reviews.llvm.org/D26267 looks like the most recent significant update
to this code that we could find. It had an additional allowance where clang
headers that were _not_ marked `textual` could be re-entered. I
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 07317bbc66d1f2d7663af3c9f04d0f6c0487ac03
171d0e299dd676ce29583e16fdf8c3e6f3dd7925 --
llvmbot wrote:
@llvm/pr-subscribers-clang
Author: Ian Anderson (ian-twilightcoder)
Changes
Once a file has been `#import`'ed, it gets stamped as if it was `#pragma once`
and will not be re-entered, even on #include. This means that any errant
#import of a file designed to be included
https://github.com/ian-twilightcoder created
https://github.com/llvm/llvm-project/pull/83660
Once a file has been `#import`'ed, it gets stamped as if it was `#pragma once`
and will not be re-entered, even on #include. This means that any errant
#import of a file designed to be included
41 matches
Mail list logo
