[PATCH] Add stopgap option -fmodule-implementation-of

[Richard Smith]

On Fri, Jul 11, 2014 at 8:42 AM, Ben Langmuir <blangmuir at apple.com> 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.

>>> 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140716/1d773d7e/attachment.html>