Add support for EDC types in connect/disconnect #827

simone-gaia · 2021-08-12T00:15:53Z

Add support for EDC types in connect/disconnect. If the EDC types are available (the user included the EDC headers) SemaGaia will generate overloads for the connect/disconnect to include EDC types:

bool incubator::connect(sensor__type&)
bool incubator::connect(sensor_t&)

Now it is possible to do farmer.incubators.connect(incubator.insert(name: "Zombies"));

Others:

Refactor code that adds connect/disconnect into its own Sema::addConnectDisconnect
Refactor code that lookup EDC types into Sema::lookupEDCClass

TODO:
farmer.incubators.connect(incubator.insert(name: "Zombies")); will not work if the EDC headers are not included, because of this "bug": https://gaiaplatform.atlassian.net/browse/GAIAPLAT-1167

daxhaw · 2021-08-12T15:09:24Z

third_party/production/TranslationEngineLLVM/clang/include/clang/Sema/Sema.h

+  /// Note: the lookup logic does not follow the Clang way of doing it (see lookupClass).
+  ///  The class name will be searched everywhere regardless of the namespace. If the class
+  ///  name appears multiple times in different namespaces the first occurrence is returned.
+  ///  This is inline with the fact that the translation process is unaware of databases.


Might want to tag this with a TODO comment and jira reference (GAIAPLAT-1028?) that if we want the translation engine to differentiate between multiple databases then we'll need to change this behavior. I've been using //TODO[GAIAPLAT-NNNN]... to mark code we need to revisit later.

daxhaw · 2021-08-12T15:14:11Z

third_party/production/TranslationEngineLLVM/clang/lib/Sema/SemaGaia.cpp

@@ -676,6 +655,77 @@ TagDecl* Sema::lookupClass(std::string className, SourceLocation loc, Scope* sco
    return classDecl;
 }

+TagDecl* Sema::lookupEDCClass(std::string className)


LLVM 101 for me: Do we not know the class type at this time? So you have to search by name? I naively though you could a) restrict the search based on whether the class inherited from edc_base_t and then b) compare the types directly instead of doing string matching. Since we're not doing a namespace lookup here, string matching should work but just curious why.

Not sure if I understand the question. I'll try to answer.

(Greg may know better, I'll try to give my rough understanding of what is going on, happy to spend some time after the sync up to discuss further).

At this point of the compilation process, we are reading tokens from the input and try to give meaning to them (Sema). What happens is that we get strings and we need to decide what they are and either assigning them to an existing class (which we need to lookup) or creating a new class at runtime (table__type).

Given this ruleset:

on_update(farmer) { cout << farmer.name << endl; }

"farmer.name" is passed into the SemaGaia::injectVariableDefinition, where it is decided if it is a table reference (getTableType) or a field reference (getFieldType):

size_t dot_position = table.find('.'); if (dot_position != string::npos) { // Called passing the "farmer" string to getTableType which will call lookupEDCClass. qualType = getTableType(table.substr(0, dot_position), loc); <== CALL THIS } else { qualType = getFieldType(table, loc); }

Within the getTableType() we need to check if the EDC type for the farmer table is available. The EDC types may not be available at all if the EDC headers are not included (IMO this is a little wonky, one day we may force the user to include them or inject them).

The Clang way to lookup names (namespaces, function, variables, classes, etc..) is what we do here, which takes into consideration context, scope etc.. https://github.com/gaia-platform/GaiaPlatform/pull/827/files#diff-c6e8da967d5b43ad73a06e39e1d2acab7dcee01bdb9badbd5911b342c48e28d2R629-R656

daxhaw · 2021-08-12T15:17:48Z

third_party/production/TranslationEngineLLVM/clang/lib/Sema/SemaGaia.cpp

+    //  ....
+
+    auto& types = Context.getTypes();
+    for (unsigned typeIdx = 0; typeIdx != types.size(); ++typeIdx)


nit: why can't you just iterate directly over the llvm::SmallVectorImpl<clang::Type*> returned from Context.getTypes()?

Whoopsy, that is indeed possible. Done.

daxhaw · 2021-08-12T15:28:37Z

third_party/production/TranslationEngineLLVM/clang/lib/Sema/SemaGaia.cpp

+    targetTypes.push_back(implicitTargetTypeDecl);
+
+    // Lookup the EDC class type (table_t)
+    string edcTableTypeName = targetTableName + "_t";


A TODO[GAIAPLAT-1168] tag would be good here to remind us to update this code to look in the catalog for the typename instead of just assuming it will always be table + "_t". (I just entered GAIAPLAT-1168 as a placeholder for this - if you know of a better item already in Jira, feel free to use that one instead and then close this one. My brief Jira search didn't yield anything related to this, however).

Done. 100% agree.

daxhaw · 2021-08-12T15:30:52Z

third_party/production/TranslationEngineLLVM/clang/lib/Sema/SemaGaia.cpp

+        targetTypes.push_back(edcTargetTypeDecl);
+    }
+
+    // Add connect/disconnect both for the implicit class and EDC class (if available).


So, just to make sure I'm clear on this, the EDC class will not be available if:

the rule author has not declared an EDC variable in their rule?

the rule author has not included the EDC generated header in their ruleset

both of the above

none of the above and I should get the number of that truck driving school

The types are introduced in include files. You don't have to declare anything to make compiler aware of a type.

What Greg said.

daxhaw · 2021-08-12T15:35:34Z

third_party/production/TranslationEngineLLVM/clang/test/Parser/gaia_connect_disconnect.cpp

+
+        // This won't work because of https://gaiaplatform.atlassian.net/browse/GAIAPLAT-1167
+        // the workaround is to have the raised_t type available.
+//        farmer.connect(raised.insert(birthdate: "2 Aug 1990"));


I saw that you entered GAIAPLAT-1167 against V1. How hard would it be to fix for Preview? I realize that explicit connect/disconnect is lower-priority than getting autoconnect/disconnect to work so I understand your priority call on this but just wondering. This falls under the "do not violate the principle of least surprise" and then the "do not introduce another paper cut". The latter, of course, leads to "death by a thousand papercuts".

I didn't do the research but likely a conversion function (similar to what was done to convert between generated types and EDC types) injection is required

How hard would it be to fix for Preview?

I don't know, yesterday I spent a fair amount of time looking into it without getting anywhere. Probably we need to do what Greg is saying. I switched it to Preview and left the triage-todo label so that we can re-evaluate this decision in the next triaging meeting.

daxhaw · 2021-08-12T15:38:08Z

third_party/production/TranslationEngineLLVM/clang/test/Parser/gaia_connect_disconnect.cpp

+        // convert incubator__type into incubator_t.
+        farmer.incubators.connect(incubator.insert(name: "Zombies"));
+
+        // Not putting any checks here because for some reason it does not work...


Regarding your comment "it does not work" - is this a new issue or related to GAIAPLAT-1167?

Unrelated, it is likely due to my ignorance about the testing framework...

daxhaw

Looks fine to me. My comments were mostly questions and a couple asking you to "tag" the code with JIRA item IDs so that we have breadcrumbs when we come back in V1 and support thins better.

Add support for EDC types in connect/disconnect

8a5f23a

simone-gaia requested review from waynelwarren, phillipov, daxhaw and fineg74 August 12, 2021 00:15

daxhaw reviewed Aug 12, 2021

View reviewed changes

daxhaw approved these changes Aug 12, 2021

View reviewed changes

Address PR feedback

f1b9448

fineg74 approved these changes Aug 12, 2021

View reviewed changes

simone-gaia merged commit f9c32fe into master Aug 12, 2021

simone-gaia deleted the rondelli-conn-disconn-edc branch August 12, 2021 19:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for EDC types in connect/disconnect #827

Add support for EDC types in connect/disconnect #827

simone-gaia commented Aug 12, 2021 •

edited

Loading

daxhaw Aug 12, 2021 •

edited

Loading

simone-gaia Aug 12, 2021

daxhaw Aug 12, 2021

simone-gaia Aug 12, 2021

daxhaw Aug 12, 2021

simone-gaia Aug 12, 2021

daxhaw Aug 12, 2021 •

edited

Loading

simone-gaia Aug 12, 2021

daxhaw Aug 12, 2021 •

edited

Loading

fineg74 Aug 12, 2021

simone-gaia Aug 12, 2021

daxhaw Aug 12, 2021 •

edited

Loading

fineg74 Aug 12, 2021

simone-gaia Aug 12, 2021

daxhaw Aug 12, 2021

simone-gaia Aug 12, 2021

daxhaw left a comment

Add support for EDC types in connect/disconnect #827

Add support for EDC types in connect/disconnect #827

Conversation

simone-gaia commented Aug 12, 2021 • edited Loading

daxhaw Aug 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxhaw Aug 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxhaw Aug 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxhaw Aug 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxhaw left a comment

Choose a reason for hiding this comment

simone-gaia commented Aug 12, 2021 •

edited

Loading

daxhaw Aug 12, 2021 •

edited

Loading

daxhaw Aug 12, 2021 •

edited

Loading

daxhaw Aug 12, 2021 •

edited

Loading

daxhaw Aug 12, 2021 •

edited

Loading