Page MenuHomePhabricator

Add -fno-modules-implicit-builds.
ClosedPublic

Authored by klimek on Dec 17 2014, 8:31 AM.

Details

Reviewers
djasper
rsmith
Summary

If this flag is set, we error out when a module build is required. This is
useful in environments where all required modules are passed via -fmodule-file.

Diff Detail

Event Timeline

klimek updated this revision to Diff 17395.Dec 17 2014, 8:31 AM
klimek retitled this revision from to Add -fno-modules-implicit-builds..
klimek updated this object.
klimek edited the test plan for this revision. (Show Details)
klimek added reviewers: rsmith, djasper.
klimek added a subscriber: Unknown Object (MLST).
rsmith edited edge metadata.Dec 17 2014, 11:39 AM

Your change and your documentation don't match. The difference is the behavior in the case where a module is not provided explicitly but is found in the module cache; your documentation says this is invalid but your code accepts it. I think the more principled approach would be to do as your documentation suggests: require all modules to be provided via explicitly-specified .pcm files, and never look in the module cache.

I would also like to bikeshed the flag name; -fno-implicit-modules seems like a better name to me. I'd also suggest renaming -fno-modules-implicit-maps to -fno-implicit-module-maps because "module map" is the term of art here, and it doesn't make sense to split this term. Grouping these flags under -fmodules seems secondary to having them make sense as flag names.

docs/Modules.rst
213–214

Please also add documentation for -fmodule-file if you're going to reference it from here. =)

include/clang/Basic/DiagnosticCommonKinds.td
84–85

This seems like the wrong message to produce -- the problem here is that the module *was* found in some loaded module map file, but a prebuilt form of that module was not provided.

klimek updated this revision to Diff 17440.Dec 18 2014, 6:13 AM
klimek edited edge metadata.

Shed bikes, more docs, cop out earlier.

The only thing not done is the renaming of the modules-implicit-maps flag, which I'd like to do in a separate patch.

docs/Modules.rst
213–214

Done.

include/clang/Basic/DiagnosticCommonKinds.td
84–85

Better?

Please add test coverage showing that we can use explicitly-specified modules via -fmodule-file=, and that we can't use implicitly-build modules that are in the cache.

lib/Frontend/CompilerInstance.cpp
1364–1368

This will also reject modules loaded via -fmodule-file=.

1376–1382

The check should go somewhere around here, after we've determined whether the user gave us a file containing this module. if (!Explicit && !ImplicitModules).

klimek updated this revision to Diff 17493.Dec 19 2014, 8:05 AM

Added tests.

lib/Frontend/CompilerInstance.cpp
1364–1368

Oh, now I understand what "Explicit" means here...

1376–1382

Done.

klimek updated this revision to Diff 17854.Jan 7 2015, 5:19 AM

Happy new Year ping (and sync).

rsmith added inline comments.Jan 14 2015, 7:19 PM
include/clang/Basic/DiagnosticCommonKinds.td
84–86

This diagnostic suggests that it'd be OK if the module didn't need to be compiled. Maybe something like "module '%0' is needed but has not been provided, and implicit use of module files is disabled"?

include/clang/Driver/Options.td
706–711

It doesn't look like you're forwarding these from the driver to the frontend (and you have no test coverage for the driver flags). The same appears to be true of -fmodules-implicit-maps, which currently doesn't seem to work as a driver option...

Also, it's conventional to only provide one of the -fblah and -fno-blah options as a -cc1 option (the non-default flag only); see OPT_fmodules_decluse for an example.

test/Modules/no-implicit-builds.cpp
4–7

Do you need the -fmodule-name= in these tests? It doesn't seem relevant to the subject matter. If you can remove that, can you also remove a.modulemap entirely and directly use b.modulemap here?

27–28

Is this CHECK useful?

klimek updated this revision to Diff 20074.Feb 17 2015, 2:48 AM

Apply review comments.

klimek updated this revision to Diff 20075.Feb 17 2015, 2:52 AM

Remove whitespace diff.

include/clang/Basic/DiagnosticCommonKinds.td
84–86

Done.

include/clang/Driver/Options.td
706–711

Done.

test/Modules/no-implicit-builds.cpp
4–7

Done. I'm a little unsure whether the test now still tests everything I want it to test...

27–28

Done.

rsmith accepted this revision.Feb 18 2015, 7:24 PM
rsmith edited edge metadata.
rsmith added inline comments.
docs/Modules.rst
216

-fmodule-file=<FILE> or similar would be clearer

lib/Driver/Tools.cpp
3928–3933

Maybe hold back on adding driver support for this flag until after it's renamed?

test/Modules/Inputs/no-implicit-builds/a.modulemap
1–5 ↗(On Diff #20075)

This file appears to be unused.

test/Modules/no-implicit-builds.cpp
16–17

Add -Rmodule-build to the command line for safety / clarity, given this.

This revision is now accepted and ready to land.Feb 18 2015, 7:24 PM
klimek updated this revision to Diff 20279.Feb 19 2015, 4:30 AM
klimek edited edge metadata.

Review comments.

klimek updated this revision to Diff 20280.Feb 19 2015, 4:32 AM

Whitespace

klimek updated this revision to Diff 20281.Feb 19 2015, 4:33 AM

More whitespace.

klimek updated this revision to Diff 20282.Feb 19 2015, 4:49 AM

Fix whitespace for real?

klimek updated this revision to Diff 20283.Feb 19 2015, 4:53 AM

And one more whitespace cleanup.

docs/Modules.rst
216

Done, using <file> to match <directory> above.

lib/Driver/Tools.cpp
3928–3933

That is an excellent point, thx.

test/Modules/Inputs/no-implicit-builds/a.modulemap
1–5 ↗(On Diff #20075)

Done.

test/Modules/no-implicit-builds.cpp
16–17

Done.

djasper closed this revision.Apr 12 2015, 1:25 AM