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 -- that's exactly what I've been trying to 
say, and demonstrated with the test -- it's not true _and_ that's a bug.

> Modular headers including non-modular headers is a special problem. If we let 
> every module enter the non-modular header (which Jan's patch does), its 
> declarations will build into multiple modules/pcm files

I'm not really sure what you're getting at in this paragraph. Is there a _new_ 
special problem introduced here?

Yes, a textual header can be built into multiple modules/pcm files -- that's 
the nature of it being textual, right? Certainly it's always be a better to 
modularize headers from the bottom-up, and avoid textually including a header 
into multiple modules. And, yes, the performance cost of duplicating the decls 
into multiple modules' pcm files can be quite significant, depending on the 
size of the header. Yet, sometimes it happens. And, when it does, the 
declaration-merging takes care of it -- for C/C++ clients. This all works (and 
is used) today, with ifndef-guarded header inclusion.

But a textually-included header shouldn't be re-entered if it's been _visibly_ 
included already in the current compilation -- including if that was exported 
from an imported module. It should only be re-entered if the previous include 
is _not_ visible in the current context. Maybe you're saying Jan's patch 
doesn't do that properly yet. (I could see that being the case, since the 
behavior mostly just falls out automatically for the ifndef-guard case, based 
on whether the guard-macro was made visible or not!)

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

Reply via email to