On Mon, Jul 28, 2014 at 2:05 PM, Ben Langmuir <[email protected]> wrote:
> > On Jul 24, 2014, at 6:58 PM, Richard Smith <[email protected]> wrote: > > On Thu, Jul 24, 2014 at 7:56 AM, Ben Langmuir <[email protected]> wrote: > >> >> On Jul 16, 2014, at 3:42 PM, Richard Smith <[email protected]> wrote: >> >> On Fri, Jul 11, 2014 at 8:42 AM, Ben Langmuir <[email protected]> >> wrote: >> >>> Hey RIchard, >>> >>> Sorry to take so long to reply to this, but I am still interested in >>> getting this stopgap into tree. >>> >> >> Sorry about the delay getting back to you! >> >> >>> Please do not add a stopgap workaround to our stable and >>> backwards-compatible driver interface; just add it to -cc1 instead. >>> >>> >>> Sure. >>> >>> I don't see any relation between the flag's name and its functionality; >>> there seems to be no reason for this to be linked to the translation unit >>> being the implementation of any particular module (and if there were, >>> that's what -fmodule-name is for). Instead, I think what you're trying to >>> specify is that a particular module is included textually for this >>> compilation. Please pick a name that suggests that functionality instead. >>> >>> >>> In the abstract I agree with this, but the use case I have is only for >>> TUs that are implementation files for a module and I know that is the only >>> time that this flag will be used by our tools. It is more useful for the >>> diagnostic to say “don’t do this in the implementation of module Foo”, >>> since that matches when the build system will be passing in this flag. >>> Given that this doesn’t go into the driver, is this still an issue? If >>> not, I can update and commit this patch, or can post it again for review if >>> you prefer :-) >>> >> >> I'm fine with this as a short-term cc1-only flag. Longer-term I think we >> need to evaluate whether we can make the import-of-same-module cases "just >> work" (I think we can), and I hope this becomes unnecessary at that point. >> >> >> r213767 >> >> >> >>> What’s unexpected to me is that changing a header whose contents are >>> not usually visible may still require rebuilding all of my .cpp files. >>> >>> module Foo { module One { header “One.h” } module Two { header >>> “Two.h” } } >>> >>> >>> >>> // One.cpp - I don’t want to rebuild when Two.h changes >>> >>> #import <Foo/One.h> >>> >>> >>> >>> Do we agree that this is unnecessary if submodules cannot >>> accidentally be affected by changes in other submodules they don’t import >>> (and we have some way to get the set of dependency files for just the >>> submodule)? >>> >> >>> >> >>> >> No, I don't agree with that. One.cpp might inline some function >>> definitions from Two.h, for instance. Or it might fail to build because it >>> declares something that conflicts with something in Two.h. >>> > >>> > >>> > I feel like I”m missing something - how is that different from One.cpp >>> having conflicts with some completely different header or module that is >>> not imported into that particular TU? >>> >>> If you import any part of a module, you have the whole module as part of >>> your translation unit, even though only some of it might be visible. Thus >>> we will diagnose your declarations that conflict with unimported portions >>> of an imported module. >>> >>> Maybe we need to have this discussion on cfe-dev at some point. I think >>> we need a driver flag to control whether clang reports headers from >>> unimported submodules as dependencies, which will allow users/build systems >>> to make the tradeoff. As for the default, I strongly feel we shouldn't >>> penalize build performance for correct code in order to guarantee that >>> these particular ODR violations get diagnosed in incremental builds. A >>> full rebuild will still see any diagnostics and the subset of errors that >>> this affects are not being diagnosed today with headers, so we’re still >>> improving. >>> >> >> Conversely, I think that we should provide a guarantee that incremental >> and full builds produce bit-for-bit identical results. As you say, it's a >> tradeoff, but note that this isn't just about ODR violation checking -- the >> incremental approach you're suggesting can generate wrong code in some >> cases (we can inline a function definition from the old version of Two.h) >> -- so if we want to support this partial-rebuild mode, we'll need to be >> /very/ careful that we don't pull in any information from an unimported >> submodule in that mode. >> >> >> Maybe you can help me understand how this would come about. In our >> documentation we say: >> >> Modules are modeled as if each submodule were a separate translation >> unit, and a module import makes names from the other translation unit >> visible >> >> >> Here’s my understanding: >> If I don’t import the submodule containing “Two.h”, then I shouldn’t get >> its definitions in my TU. >> > > You get its definitions in your *program*. If you import any part of a > module, the entire module is part of your program. Example: > > > Okay, but that’s just more consistency checking, ins’t it? If I import > Module1.B, but not Module1.A (or Module2.C) I don’t want to see “f” in my > exported symbols. > I think you're saying that it would in principle be possible for us to accept the example I gave? It probably would, but the fact that we reject it right now is a feature, not a bug. > Module1.A: > int f(int); > > Module1.B: > extern int n; > > Module2.C: > import Module1.B; > void f(int); // error, conflicting return type > > If I have an inline declaration for a function in Two, then I still need >> to have a definition in my own TU because of inline. If I have a >> non-inline decl, then Two can’t have an inline decl and if it has a >> definition for the function not marked inline then having that definition >> show up in my TU would lead to multiple definitions if Two is imported >> somewhere else. >> > > You can get into this situation with C++ templates. You might only be able > to see a declaration of a template, where another submodule provides a > definition that is hidden but still available for inlining. This doesn't > violate any language rule as long as there's an explicit instantiation of > the template somewhere. > > > If I don’t see a definition in my TU, how can I use the template in a way > affected by inlining? > You do "see" a definition in your TU, for some value of "see". That definition *is* imported, and is known about by the compiler; we just give you an error if you try to use it. CodeGen is still able to emit it. This is necessary to support entities that are imported by a module but not re-exported. Consider this: Module X: inline int f() { return 0; } Module Y: import X; // not re-exported inline int g() { return f(); } Z.cc: import Y; int k = g(); In Z.cc, we are *required* to emit the body of 'f', even though you can't "see" it. And entities in X are treated just like entities in an unimported submodule of Y. > I may not have an instantiation of a template, but I still need to see its > definition. If its definition changes, that would require rebuilding the > other TU that has the instantiation. I’m probably being thick, but I still > don’t see the issue here. > > > You can also get into this situation with the C99 inline rules, where you > don't have to define an 'inline' function in every translation unit. > > > Did this change in C11, or am I misreading this? > 6.7.4.7: For a function with external linkage, the following restrictions > apply: If a function is declared with an inline function specifier, then it > shall also be defined in the same translation unit. > That rule applies only if the function is declared with the 'inline' specifier in that translation unit. Example: Module X.A: extern int f(void); // ok, no 'inline', no definition required in this TU Module X.B: inline int f() { return 0; } // ok, definition main.cc: import X.A; int main() { return f(); } In this setup, f() might get inlined into main, even though the definition is not visible. (FWIW, I expect we'll also generate wrong code in this case, because we'll emit a strong definition of 'f' from every TU that imports X; conversely, if X.A and X.B are split into separate top-level modules, then a TU that imports both will not emit a strong definition of 'f'.)
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
