[Compiler] Implement module loading.
ClosedPublic

Authored by akreuzkamp on Sep 8 2015, 12:40 PM.

Details

Summary

This branch implements an abstract class AbstractModuleLoader with
factory registration. On top of it, it implements a
JavaScriptModuleLoader, that loads qml modules implemented in js.
It provides a static method ModuleLoading::loadModule that will look
for an appropriate loader and will (asynchronously) load the module.

This branch removes symbol table, which turned out, not to make any
sense.

Diff Detail

Repository
R18 QMLWeb
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
akreuzkamp updated this revision to Diff 826.Sep 8 2015, 12:40 PM
akreuzkamp retitled this revision from to [Compiler] Implement module loading..
akreuzkamp edited the test plan for this revision. (Show Details)
akreuzkamp added a reviewer: jangmarker.
akreuzkamp added a task: Restricted Maniphest Task.
akreuzkamp updated this object.
jangmarker edited edge metadata.Sep 8 2015, 2:51 PM

Thanks for that work and the thousand revamps, I like it's shape very much.

I didn't review the visitors very closely, just got an overview of what they do.

src/qmljsc/error.cpp
2 ↗(On Diff #826)

I'm not sure, but I guess that could be corrected in this merge request

src/qmljsc/error.h
28 ↗(On Diff #826)

nitpicking, but I think this should be a separate merge request

src/qmljsc/ir/file.cpp
147 ↗(On Diff #826)

Ambitious? You probably mean ambiguous

src/qmljsc/ir/file.h
35 ↗(On Diff #826)

From my understanding this struct's coupling is more deep with Module than with File. I would suggest to either move it to Module or into its own h/cpp.

75 ↗(On Diff #826)

This class seems to be quite reusable. Is it sensible to move that out of File's header?

108 ↗(On Diff #826)

please rename to m_importedModules or alike

src/qmljsc/ir/module.cpp
2 ↗(On Diff #826)

see above

src/qmljsc/ir/module.h
2 ↗(On Diff #826)

Please add the project sentence:
Qml.js Compiler - a QML to JS compiler bringing QML's power to the web.

80 ↗(On Diff #826)

please rename to m_importDescription

81 ↗(On Diff #826)

maybe loadingState?

82 ↗(On Diff #826)

maybe rename to m_loadingFinished

src/qmljsc/moduleloading/abstractmoduleloader.cpp
2 ↗(On Diff #826)

project's statement is missing

src/qmljsc/moduleloading/abstractmoduleloader.h
2 ↗(On Diff #826)

project's statement missing, see above

33 ↗(On Diff #826)

the documentation seems to be outdated

38 ↗(On Diff #826)

it changed to ModuleLoading::loadModule, I think?

49 ↗(On Diff #826)

the documentation is incomplete

59 ↗(On Diff #826)

Is it possible to redesign your API to make that dependency strict by allowing construction of an AbstractModuleLoader only with an IR::Module pointer?

src/qmljsc/moduleloading/javascriptmoduleloader.cpp
2 ↗(On Diff #826)

project's statement is missing

225 ↗(On Diff #826)

please use the corresponding enumeration AST::QSOperator::OP

301 ↗(On Diff #826)

please use the enumeration, see above

342 ↗(On Diff #826)

see above

421 ↗(On Diff #826)

please either fix that todo or add a task on Phabricator

439 ↗(On Diff #826)

is arg not in the same column on purpose?

442 ↗(On Diff #826)

Maybe we could add a TODO for this as well on Phabricator?

443 ↗(On Diff #826)

That uses Compiler's singleton mechanism that we wanted to get rid of.

A mechanism we could use instead:
register the include paths on ModuleLoading and let ModuleLoading pass them to the module loaders, either by constructor or setter (I'd prefer constructor)

What do you think of that?

src/qmljsc/moduleloading/javascriptmoduleloader.h
2 ↗(On Diff #826)

project's statement is missing

src/qmljsc/moduleloading/moduleloading.cpp
2 ↗(On Diff #826)

see above

61 ↗(On Diff #826)

maybe rename factory to createModuleLoader or create, that makes line 62 better readable: loader = createModuleLoader() or loader = create();

62 ↗(On Diff #826)

if decide to apply the API change to construct a loader only with a module, that could even read like:
loader = createFor(module);

src/qmljsc/moduleloading/moduleloading.h
2 ↗(On Diff #826)

see above

56 ↗(On Diff #826)

s_moduleLoaders are actually factories, I'd propose to rename it to s_moduleLoaderFactories

tests/auto/qmljsc/testmodules.cpp
36 ↗(On Diff #826)

the test's name is wrong

42 ↗(On Diff #826)

I found QSKIP which we can use to disable tests (http://doc.qt.io/qt-5/qtest.html#QSKIP)
that will show up in the log and we don't forget to reenable it later

if the test should be removed, please ignore me and remove it :)

53 ↗(On Diff #826)

this seems to be unused

58 ↗(On Diff #826)

this uses the compiler's singleton mechanism that we wanted to get rid of.

162 ↗(On Diff #826)

what should that do? is it a leftover?

jangmarker added inline comments.Sep 8 2015, 3:26 PM
src/qmljsc/moduleloading/javascriptmoduleloader.cpp
225 ↗(On Diff #826)

sorry, it is QSOperator::OP

akreuzkamp updated this revision to Diff 830.Sep 8 2015, 8:02 PM
akreuzkamp edited edge metadata.
  • [Compiler] Correct some copyright headers.
  • [Compiler] Renaming, moving around and documenting
src/qmljsc/ir/file.cpp
147 ↗(On Diff #826)

This type name is very ambitious! It wants to get famous. Like a celebrity. That's why it appears in multiple modules already.

src/qmljsc/ir/file.h
35 ↗(On Diff #826)

This is only because currently we only have ModuleImports implemented. But, yes, I'll move it into it's own header file.

src/qmljsc/moduleloading/javascriptmoduleloader.cpp
443 ↗(On Diff #826)

I'd prefer to do that later on, in an own feature branch dedicated to getting rid of the Compiler singleton, to not mix two topics into one branch.

tests/auto/qmljsc/testmodules.cpp
53 ↗(On Diff #826)

No, it's not. It's the singleton, we use later (using the global name 'compiler'). We need to create it somewhere.

58 ↗(On Diff #826)

As written earlier, I'd prefer to do that later, separately.

162 ↗(On Diff #826)

It should check, whether the type of baked is a bool, but we need the qtqml module with the basic types for that. That's why I disabled the whole test.

jangmarker added inline comments.
src/qmljsc/error.cpp
3 ↗(On Diff #830)

q :D

src/qmljsc/moduleloading/abstractmoduleloader.h
34 ↗(On Diff #830)

this is still outdated

src/qmljsc/moduleloading/javascriptmoduleloader.cpp
444 ↗(On Diff #830)

okay, that's fine to me, so this is part of T486

akreuzkamp updated this revision to Diff 831.Sep 8 2015, 8:43 PM
  • [Compiler] Renaming, moving around and documenting
akreuzkamp updated this revision to Diff 832.Sep 8 2015, 8:51 PM
  • [Compiler] Renaming, moving around and documenting
jangmarker accepted this revision.Sep 8 2015, 8:53 PM
jangmarker edited edge metadata.

great :-)

This revision is now accepted and ready to land.Sep 8 2015, 8:53 PM
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.