more code review using github compiler runs

libraries/entities/src/EntityEditFilters.cpp

@@ -79,7 +79,7 @@ bool EntityEditFilters::filter(glm::vec3& position, EntityItemProperties& proper
             auto oldProperties = propertiesIn.getDesiredProperties();
             auto specifiedProperties = propertiesIn.getChangedProperties();
             propertiesIn.setDesiredProperties(specifiedProperties);
-            ScriptValue inputValues = propertiesIn.copyToScriptValue(filterData.engine, false, true, true);
+            ScriptValue inputValues = propertiesIn.copyToScriptValue(filterData.engine.get(), false, true, true);
             propertiesIn.setDesiredProperties(oldProperties);
 
             auto in = QJsonValue::fromVariant(inputValues.toVariant()); // grab json copy now, because the inputValues might be side effected by the filter.
@@ -91,7 +91,7 @@ bool EntityEditFilters::filter(glm::vec3& position, EntityItemProperties& proper
             // get the current properties for then entity and include them for the filter call
             if (existingEntity && filterData.wantsOriginalProperties) {
                 auto currentProperties = existingEntity->getProperties(filterData.includedOriginalProperties);
-                ScriptValue currentValues = currentProperties.copyToScriptValue(filterData.engine, false, true, true);
+                ScriptValue currentValues = currentProperties.copyToScriptValue(filterData.engine.get(), false, true, true);
                 args << currentValues;
             }
 
@@ -101,17 +101,17 @@ bool EntityEditFilters::filter(glm::vec3& position, EntityItemProperties& proper
                 auto zoneEntity = _tree->findEntityByEntityItemID(id);
                 if (zoneEntity) {
                     auto zoneProperties = zoneEntity->getProperties(filterData.includedZoneProperties);
-                    ScriptValue zoneValues = zoneProperties.copyToScriptValue(filterData.engine, false, true, true);
+                    ScriptValue zoneValues = zoneProperties.copyToScriptValue(filterData.engine.get(), false, true, true);
 
                     if (filterData.wantsZoneBoundingBox) {
                         bool success = true;
                         AABox aaBox = zoneEntity->getAABox(success);
                         if (success) {
                             ScriptValue boundingBox = filterData.engine->newObject();
-                            ScriptValue bottomRightNear = vec3ToScriptValue(filterData.engine, aaBox.getCorner());
+                            ScriptValue bottomRightNear = vec3ToScriptValue(filterData.engine.get(), aaBox.getCorner());
-                            ScriptValue topFarLeft = vec3ToScriptValue(filterData.engine, aaBox.calcTopFarLeft());
+                            ScriptValue topFarLeft = vec3ToScriptValue(filterData.engine.get(), aaBox.calcTopFarLeft());
-                            ScriptValue center = vec3ToScriptValue(filterData.engine, aaBox.calcCenter());
+                            ScriptValue center = vec3ToScriptValue(filterData.engine.get(), aaBox.calcCenter());
-                            ScriptValue boundingBoxDimensions = vec3ToScriptValue(filterData.engine, aaBox.getDimensions());
+                            ScriptValue boundingBoxDimensions = vec3ToScriptValue(filterData.engine.get(), aaBox.getDimensions());
                             boundingBox.setProperty("brn", bottomRightNear);
                             boundingBox.setProperty("tfl", topFarLeft);
                             boundingBox.setProperty("center", center);
@@ -169,10 +169,6 @@ bool EntityEditFilters::filter(glm::vec3& position, EntityItemProperties& proper
 
 void EntityEditFilters::removeFilter(EntityItemID entityID) {
     QWriteLocker writeLock(&_lock);
-    FilterData filterData = _filterDataMap.value(entityID);
-    if (filterData.valid()) {
-        delete filterData.engine;
-    }
     _filterDataMap.remove(entityID);
 }
 
@@ -273,7 +269,7 @@ void EntityEditFilters::scriptRequestFinished(EntityItemID entityID) {
             if (!hadUncaughtExceptions(*engine, urlString)) {
                 // put the engine in the engine map (so we don't leak them, etc...)
                 FilterData filterData;
-                filterData.engine = engine.get();
+                filterData.engine = engine;
                 filterData.rejectAll = false;
                 
                 // define the uncaughtException function

libraries/entities/src/EntityEditFilters.h

@@ -43,10 +43,10 @@ public:
         bool wantsZoneBoundingBox { false };
 
         std::function<bool()> uncaughtExceptions;
-        ScriptEngine* engine;
+        ScriptEnginePointer engine;
         bool rejectAll;
         
-        FilterData(): engine(nullptr), rejectAll(false) {};
+        FilterData(): rejectAll(false) {};
         bool valid() { return (rejectAll || (engine != nullptr && filterFn.isFunction() && uncaughtExceptions)); }
     };
 

libraries/midi/CMakeLists.txt

@@ -1,3 +1,4 @@
 set(TARGET_NAME midi)
 setup_hifi_library(Network)
 link_hifi_libraries(shared networking)
+include_hifi_library_headers(script-engine)

libraries/midi/src/Midi.cpp

@@ -15,6 +15,9 @@
 
 #include <QtCore/QLoggingCategory>
 
+#include <ScriptEngine.h>
+#include <ScriptManager.h>
+
 #if defined Q_OS_WIN32
 #include "Windows.h"
 #endif
@@ -131,6 +134,14 @@ void CALLBACK MidiInProc(HMIDIIN hMidiIn, UINT wMsg, DWORD_PTR dwInstance, DWORD
     }
 }
 
+STATIC_SCRIPT_INITIALIZER(+[](ScriptManager* manager) {
+    auto scriptEngine = manager->engine().data();
+
+    scriptEngine->registerGlobalObject("Midi", DependencyManager::get<Midi>().data());
+});
+
+
+
 void CALLBACK MidiOutProc(HMIDIOUT hmo, UINT wMsg, DWORD_PTR dwInstance, DWORD_PTR dwParam1, DWORD_PTR dwParam2) {
     switch (wMsg) {
         case MOM_OPEN:

libraries/script-engine/src/ScriptContext.h

@@ -43,6 +43,9 @@ public:
     virtual QString functionName() const = 0;
     virtual FunctionType functionType() const = 0;
     virtual int lineNumber() const = 0;
+
+protected:
+    ~ScriptFunctionContext() {} // prevent explicit deletion of base class
 };
 
 /// [ScriptInterface] Provides an engine-independent interface for QScriptContext
@@ -58,6 +61,9 @@ public:
     virtual ScriptValue thisObject() const = 0;
     virtual ScriptValue throwError(const QString& text) = 0;
     virtual ScriptValue throwValue(const ScriptValue& value) = 0;
+
+protected:
+    ~ScriptContext() {}  // prevent explicit deletion of base class
 };
 
 #endif // hifi_ScriptContext_h

libraries/script-engine/src/ScriptEngine.h

@@ -136,6 +136,9 @@ public: // not for public use, but I don't like how Qt strings this along with p
     virtual ScriptValue create(int type, const void* ptr) = 0;
     virtual bool convert(const ScriptValue& value, int type, void* ptr) = 0;
     virtual void registerCustomType(int type, MarshalFunction mf, DemarshalFunction df, const ScriptValue& prototype) = 0;
+
+protected:
+    ~ScriptEngine() {}  // prevent explicit deletion of base class
 };
 Q_DECLARE_OPERATORS_FOR_FLAGS(ScriptEngine::QObjectWrapOptions);
 

libraries/script-engine/src/ScriptManager.cpp

@@ -57,7 +57,6 @@
 
 #include <Profile.h>
 
-#include "../../midi/src/Midi.h"        // FIXME why won't a simpler include work?
 #include "MIDIEvent.h"
 
 #include "SettingHandle.h"
@@ -675,8 +674,6 @@ void ScriptManager::init() {
     scriptRegisterMetaType(scriptEngine, externalResourceBucketToScriptValue, externalResourceBucketFromScriptValue);
     scriptEngine->registerEnum("Script.ExternalPaths", QMetaEnum::fromType<ExternalResource::Bucket>());
 
-    scriptEngine->registerGlobalObject("Midi", DependencyManager::get<Midi>().data());
-
     scriptEngine->registerGlobalObject("Quat", &_quatLibrary);
     scriptEngine->registerGlobalObject("Vec3", &_vec3Library);
     scriptEngine->registerGlobalObject("Mat4", &_mat4Library);

libraries/script-engine/src/ScriptProgram.h

@@ -28,6 +28,9 @@ public:
     virtual ScriptSyntaxCheckResultPointer checkSyntax() const = 0;
     virtual QString fileName() const = 0;
     virtual QString sourceCode() const = 0;
+
+protected:
+    ~ScriptProgram() {}  // prevent explicit deletion of base class
 };
 
 /// [ScriptInterface] Provides an engine-independent interface for QScriptSyntaxCheckResult
@@ -45,6 +48,9 @@ public:
     virtual int errorLineNumber() const = 0;
     virtual QString errorMessage() const = 0;
     virtual State state() const = 0;
+
+protected:
+    ~ScriptSyntaxCheckResult() {}  // prevent explicit deletion of base class
 };
 
 #endif // hifi_ScriptProgram_h

libraries/script-engine/src/ScriptValue.cpp

@@ -14,7 +14,7 @@
 #include "ScriptEngineLogging.h"
 
 
-class ScriptValueProxyNull : public ScriptValueProxy {
+class ScriptValueProxyNull final : public ScriptValueProxy {
 public:
     virtual void release() override;
     virtual ScriptValueProxy* copy() const override;
@@ -43,7 +43,7 @@ public:
                                  const ScriptValue::ResolveFlags& mode = ScriptValue::ResolvePrototype) const override;
     virtual ScriptValue property(quint32 arrayIndex,
                                  const ScriptValue::ResolveFlags& mode = ScriptValue::ResolvePrototype) const override;
-    virtual void setData(const ScriptValue& val);
+    virtual void setData(const ScriptValue& val) override;
     virtual void setProperty(const QString& name,
                              const ScriptValue& value,
                              const ScriptValue::PropertyFlags& flags = ScriptValue::KeepExistingFlags) override;

libraries/script-engine/src/ScriptValue.h

@@ -163,6 +163,9 @@ public:
     virtual quint32 toUInt32() const = 0;
     virtual QVariant toVariant() const = 0;
     virtual QObject* toQObject() const = 0;
+
+protected:
+    ~ScriptValueProxy() {}  // prevent explicit deletion of base class
 };
 
 // the second template parameter is used to defer evaluation of calls to the engine until ScriptEngine isn't forward-declared
@@ -178,155 +181,193 @@ void ScriptValue::setProperty(quint32 arrayIndex, const TYP& value, const Proper
 }
 
 ScriptValue::ScriptValue(const ScriptValue& src) : _proxy(src.ptr()->copy()) {
+    Q_ASSERT(_proxy != nullptr);
 }
 
 ScriptValue::~ScriptValue() {
+    Q_ASSERT(_proxy != nullptr);
     _proxy->release();
 }
 
 ScriptValue& ScriptValue::operator=(const ScriptValue& other) {
+    Q_ASSERT(_proxy != nullptr);
     _proxy->release();
     _proxy = other.ptr()->copy();
     return *this;
 }
 
 ScriptValue ScriptValue::call(const ScriptValue& thisObject, const ScriptValueList& args) const {
+    Q_ASSERT(_proxy != nullptr);
     return _proxy->call(thisObject, args);
 }
 
 ScriptValue ScriptValue::call(const ScriptValue& thisObject, const ScriptValue& arguments) const {
+    Q_ASSERT(_proxy != nullptr);
     return _proxy->call(thisObject, arguments);
 }
 
 ScriptValue ScriptValue::construct(const ScriptValueList& args) const {
+    Q_ASSERT(_proxy != nullptr);
     return _proxy->construct(args);
 }
 
 ScriptValue ScriptValue::construct(const ScriptValue& arguments) const {
+    Q_ASSERT(_proxy != nullptr);
     return _proxy->construct(arguments);
 }
 
 ScriptValue ScriptValue::data() const {
+    Q_ASSERT(_proxy != nullptr);
     return _proxy->data();
 }
 
 ScriptEnginePointer ScriptValue::engine() const {
+    Q_ASSERT(_proxy != nullptr);
     return _proxy->engine();
 }
 
 bool ScriptValue::equals(const ScriptValue& other) const {
+    Q_ASSERT(_proxy != nullptr);
     return _proxy->equals(other);
 }
 
 bool ScriptValue::isArray() const {
+    Q_ASSERT(_proxy != nullptr);
     return _proxy->isArray();
 }
 
 bool ScriptValue::isBool() const {
+    Q_ASSERT(_proxy != nullptr);
     return _proxy->isBool();
 }
 
 bool ScriptValue::isError() const {
+    Q_ASSERT(_proxy != nullptr);
     return _proxy->isError();
 }
 
 bool ScriptValue::isFunction() const {
+    Q_ASSERT(_proxy != nullptr);
     return _proxy->isFunction();
 }
 
 bool ScriptValue::isNumber() const {
+    Q_ASSERT(_proxy != nullptr);
     return _proxy->isNumber();
 }
 
 bool ScriptValue::isNull() const {
+    Q_ASSERT(_proxy != nullptr);
     return _proxy->isNull();
 }
 
 bool ScriptValue::isObject() const {
+    Q_ASSERT(_proxy != nullptr);
     return _proxy->isObject();
 }
 
 bool ScriptValue::isString() const {
+    Q_ASSERT(_proxy != nullptr);
     return _proxy->isString();
 }
 
 bool ScriptValue::isUndefined() const {
+    Q_ASSERT(_proxy != nullptr);
     return _proxy->isUndefined();
 }
 
 bool ScriptValue::isValid() const {
+    Q_ASSERT(_proxy != nullptr);
     return _proxy->isValid();
 }
 
 bool ScriptValue::isVariant() const {
+    Q_ASSERT(_proxy != nullptr);
     return _proxy->isVariant();
 }
 
 ScriptValueIteratorPointer ScriptValue::newIterator() const {
+    Q_ASSERT(_proxy != nullptr);
     return _proxy->newIterator();
 }
 
 ScriptValue ScriptValue::property(const QString& name, const ResolveFlags& mode) const {
+    Q_ASSERT(_proxy != nullptr);
     return _proxy->property(name, mode);
 }
 
 ScriptValue ScriptValue::property(quint32 arrayIndex, const ResolveFlags& mode) const {
+    Q_ASSERT(_proxy != nullptr);
     return _proxy->property(arrayIndex, mode);
 }
 
 void ScriptValue::setData(const ScriptValue& val) {
+    Q_ASSERT(_proxy != nullptr);
     return _proxy->setData(val);
 }
 
 void ScriptValue::setProperty(const QString& name, const ScriptValue& value, const PropertyFlags& flags) {
+    Q_ASSERT(_proxy != nullptr);
     return _proxy->setProperty(name, value, flags);
 }
 
 void ScriptValue::setProperty(quint32 arrayIndex, const ScriptValue& value, const PropertyFlags& flags) {
+    Q_ASSERT(_proxy != nullptr);
     return _proxy->setProperty(arrayIndex, value, flags);
 }
 
 void ScriptValue::setPrototype(const ScriptValue& prototype) {
+    Q_ASSERT(_proxy != nullptr);
     return _proxy->setPrototype(prototype);
 }
 
 bool ScriptValue::strictlyEquals(const ScriptValue& other) const {
+    Q_ASSERT(_proxy != nullptr);
     return _proxy->strictlyEquals(other);
 }
 
 bool ScriptValue::toBool() const {
+    Q_ASSERT(_proxy != nullptr);
     return _proxy->toBool();
 }
 
 qint32 ScriptValue::toInt32() const {
+    Q_ASSERT(_proxy != nullptr);
     return _proxy->toInt32();
 }
 
 double ScriptValue::toInteger() const {
+    Q_ASSERT(_proxy != nullptr);
     return _proxy->toInteger();
 }
 
 double ScriptValue::toNumber() const {
+    Q_ASSERT(_proxy != nullptr);
     return _proxy->toNumber();
 }
 
 QString ScriptValue::toString() const {
+    Q_ASSERT(_proxy != nullptr);
     return _proxy->toString();
 }
 
 quint16 ScriptValue::toUInt16() const {
+    Q_ASSERT(_proxy != nullptr);
     return _proxy->toUInt16();
 }
 
 quint32 ScriptValue::toUInt32() const {
+    Q_ASSERT(_proxy != nullptr);
     return _proxy->toUInt32();
 }
 
 QVariant ScriptValue::toVariant() const {
+    Q_ASSERT(_proxy != nullptr);
     return _proxy->toVariant();
 }
 
 QObject* ScriptValue::toQObject() const {
+    Q_ASSERT(_proxy != nullptr);
     return _proxy->toQObject();
 }
 

libraries/script-engine/src/ScriptValueIterator.h

@@ -31,6 +31,9 @@ public:
     virtual QString name() const = 0;
     virtual void next() = 0;
     virtual ScriptValue value() const = 0;
+
+protected:
+    ~ScriptValueIterator() {}  // prevent explicit deletion of base class
 };
 
 #endif  // hifi_ScriptValueIterator_h

libraries/script-engine/src/Scriptable.cpp

@@ -11,14 +11,12 @@
 
 #include "Scriptable.h"
 
-#include <QtCore/QThreadStorage>
+static thread_local ScriptContext* ScriptContextStore;
-
-static QThreadStorage<ScriptContext*> ScriptContextStore;
 
 ScriptContext* Scriptable::context() {
-    return ScriptContextStore.localData();
+    return ScriptContextStore;
 }
 
 void Scriptable::setContext(ScriptContext* context) {
-    ScriptContextStore.setLocalData(context);
+    ScriptContextStore = context;
 }

libraries/script-engine/src/qtscript/ScriptContextQtAgent.h

@@ -24,17 +24,17 @@ class QScriptValue;
 class ScriptContextQtWrapper;
 using ScriptContextQtPointer = QSharedPointer<ScriptContextQtWrapper>;
 
-class ScriptContextQtAgent : public QScriptEngineAgent {
+class ScriptContextQtAgent final : public QScriptEngineAgent {
 public: // construction
     inline ScriptContextQtAgent(ScriptEngineQtScript* engine, QScriptEngineAgent* prevAgent) :
         QScriptEngineAgent(engine), _engine(engine), _prevAgent(prevAgent) {}
     virtual ~ScriptContextQtAgent() {}
 
 public: // QScriptEngineAgent implementation
-    virtual void contextPop();
+    virtual void contextPop() override;
-    virtual void contextPush();
+    virtual void contextPush() override;
-    virtual void functionEntry(qint64 scriptId);
+    virtual void functionEntry(qint64 scriptId) override;
-    virtual void functionExit(qint64 scriptId, const QScriptValue& returnValue);
+    virtual void functionExit(qint64 scriptId, const QScriptValue& returnValue) override;
 
 private: // storage
     bool _contextActive = false;

libraries/script-engine/src/qtscript/ScriptContextQtWrapper.h

@@ -25,9 +25,9 @@ class QScriptContext;
 class ScriptEngineQtScript;
 
 /// [QtScript] Implements ScriptContext for QtScript and translates calls for QScriptContextInfo
-class ScriptContextQtWrapper : public ScriptContext {
+class ScriptContextQtWrapper final : public ScriptContext {
 public: // construction
-    inline ScriptContextQtWrapper(ScriptEngineQtScript* engine, QScriptContext* context) : _engine(engine), _context(context) {}
+    inline ScriptContextQtWrapper(ScriptEngineQtScript* engine, QScriptContext* context) : _context(context) , _engine(engine) {}
     static ScriptContextQtWrapper* unwrap(ScriptContext* val);
     inline QScriptContext* toQtValue() const { return _context; }
 

libraries/script-engine/src/qtscript/ScriptEngineQtScript.h

@@ -41,7 +41,9 @@ using ScriptContextQtPointer = QSharedPointer<ScriptContextQtWrapper>;
 Q_DECLARE_METATYPE(ScriptEngineQtScriptPointer);
 
 /// [QtScript] Implements ScriptEngine for QtScript and translates calls for QScriptEngine
-class ScriptEngineQtScript : public QScriptEngine, public ScriptEngine, public QEnableSharedFromThis<ScriptEngineQtScript> {
+class ScriptEngineQtScript final : public QScriptEngine,
+                                   public ScriptEngine,
+                                   public QEnableSharedFromThis<ScriptEngineQtScript> {
     Q_OBJECT
 
 public:  // ScriptEngine implementation

libraries/script-engine/src/qtscript/ScriptProgramQtWrapper.h

@@ -22,7 +22,7 @@
 #include "ScriptEngineQtScript.h"
 
 /// [QtScript] Implements ScriptProgram for QtScript and translates calls for QScriptProgram
-class ScriptProgramQtWrapper : public ScriptProgram {
+class ScriptProgramQtWrapper final : public ScriptProgram {
 public: // construction
     inline ScriptProgramQtWrapper(ScriptEngineQtScript* engine, const QScriptProgram& value) :
         _engine(engine), _value(value) {}
@@ -41,7 +41,7 @@ private: // storage
     QScriptProgram _value;
 };
 
-class ScriptSyntaxCheckResultQtWrapper : public ScriptSyntaxCheckResult {
+class ScriptSyntaxCheckResultQtWrapper final : public ScriptSyntaxCheckResult {
 public: // construction
     inline ScriptSyntaxCheckResultQtWrapper(QScriptSyntaxCheckResult&& value) :
         _value(std::move(value)) {}

libraries/script-engine/src/qtscript/ScriptValueIteratorQtWrapper.h

@@ -23,7 +23,7 @@
 #include "ScriptValueQtWrapper.h"
 
 /// [QtScript] Implements ScriptValueIterator for QtScript and translates calls for QScriptValueIterator
-class ScriptValueIteratorQtWrapper : public ScriptValueIterator {
+class ScriptValueIteratorQtWrapper final : public ScriptValueIterator {
 public: // construction
     inline ScriptValueIteratorQtWrapper(ScriptEngineQtScript* engine, const ScriptValue& object) :
         _engine(engine), _value(ScriptValueQtWrapper::fullUnwrap(engine, object)) {}

libraries/script-engine/src/qtscript/ScriptValueQtWrapper.h

@@ -24,7 +24,7 @@
 #include "ScriptEngineQtScript.h"
 
 /// [QtScript] Implements ScriptValue for QtScript and translates calls for QScriptValue
-class ScriptValueQtWrapper : public ScriptValueProxy {
+class ScriptValueQtWrapper final : public ScriptValueProxy {
 public: // construction
     inline ScriptValueQtWrapper(ScriptEngineQtScript* engine, const QScriptValue& value) :
         _engine(engine), _value(value) {}