[GAIAPLAT-672] Add type safety to connect/disconnect params #816

simone-gaia · 2021-08-08T19:04:39Z

The purpose of this change is to make connect/disconnect be added to tables/links only when necessary and to accept only the appropriate parameter types.

Given a table, connect/disconnect are added for every unique outgoing link. Unique means that the given table has only one link with the target table thus connect/disconnect are known.
A field is added to the table for every outgoing link. Each of these fields will expose a connect/disconnect method with the appropriate type.
Dynamic types (table__type) are generated only once and reused when necessary.
GaiaFieldLValueAttr is no longer attached to the class declaration but to the variable declaration.
Simplified the parameter specification for the SemaGaia::addMethod method.

What is missing:

connect/disconnect do not accept EDC types
the whole gaiat stuff.

NB: This PR is kinda small, but it costed blood, sweat, and tears...

TC Link: http://segate2:8111/buildConfiguration/GaiaPlatform_ProductionGaiaLLVMTestsGdev/15037

- Make SemaGaia to re-use table types if already defiend.

…into rondelli-conn-disconn-params-type

This reverts commit fe10491.

LaurentiuCristofor · 2021-08-09T07:21:47Z

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

+  QualType getLinkType(const std::string& linkName, const std::string& from_table, const std::string& to_table, SourceLocation loc);
+  void addMethod(IdentifierInfo *name, DeclSpec::TST retValType, SmallVector<QualType, 8> parameterTypes,
+                 AttributeFactory &attrFactory, ParsedAttributes &attrs, RecordDecl *RD,
+    SourceLocation loc, bool isVariadic = false, ParsedType returnType = nullptr);


This line is indented differently from the one above.

LaurentiuCristofor · 2021-08-09T07:25:21Z

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

-}
+          if (memberExp != nullptr)
+          {
+              exp = dyn_cast<DeclRefExpr>(memberExp->getBase());


Don't know the class relationships here, but can't exp be null after this statement? If it can't be, it would help to add an assert to that effect.

According to the MemeberExpr constructor, it can't be (it would throw SegFault if Base was nullptr). There is setBase() method that can potentially allow to set it to nullptr. Not a problem for us since we check 11340 if exp != nullptr.

If exp == nullptr it is ok to skip the next if statement.

Base may be non nullptr but it may be not of DeclRefExpr type so dyn_cast may return nullptr

LaurentiuCristofor · 2021-08-09T07:25:47Z

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

-        ValueDecl *decl = exp->getDecl();
+      if (exp == nullptr)
+      {
+          MemberExpr* memberExp = dyn_cast<MemberExpr>(E);


Are these dyn_cast aliases for dynamic_cast or are they something different?

It has the same semantic but works differently, it does not require v-tables. https://stackoverflow.com/a/12083335/1214125.

It seems the standard across the LLVM/Clang codebase hence I'm sticking to it. I honestly don't know what is the advantage of not using v-tables, I assume is performance...

LaurentiuCristofor · 2021-08-09T07:26:48Z

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

@@ -497,10 +499,39 @@ void Sema::addMethod(IdentifierInfo* name, DeclSpec::TST retValType, DeclaratorC

    DS.Finish(*this, getPrintingPolicy());

+    SmallVector<DeclaratorChunk::ParamInfo, 8> paramsInfo;


Can this 8 be declared as a constant/constexpr? That would help clarify its semantics too.

I thought of that, but I noticed that it is not done anywhere in the LLVM/Clang codebase. Knowing how SmallVector works, you know it means that you expect at most 8 elements in the vector (which will be stack-allocated). If you add more elements they will be heap-allocated.

You will be filling a vector with data so it won't be able to declare it const

LaurentiuCristofor · 2021-08-09T07:30:23Z

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

+             iter++)
+        {
+            NamedDecl* foundDecl = iter.getDecl();
+            classDeclaration = cast<TagDecl>(previousDeclLookup.getFoundDecl());


Given the widespread use of decl in this code, we should be consistent and avoid declaration altogether.

Sounds good.

LaurentiuCristofor · 2021-08-09T07:38:58Z

third_party/production/TranslationEngineLLVM/clang/test/Parser/barn_storage.ddl

@@ -1,27 +1,27 @@
-CREATE DATABASE if not exists incubator;
+create database if not exists incubator;


I think the original idea was to use uppercase for keywords and lowercase for user-specified values. That got broken when if not exists got added, but I still think it's a nicer SQL-style than having everything lowercase. Of course, we'd have to be consistent about it.

@vDonGlover : we probably want to define a SQL/DDL style and use it consistently across all our examples.

I'm fine with it, as long as we are consistent.

LaurentiuCristofor

I can't follow the LLVM code logic, but I left some comments on some small issues. Perhaps the most important is deciding on a style to follow for DDL and SQL files (can be shared across both unless we decide to make our DDL fundamentally different from SQL).

daxhaw · 2021-08-09T18:20:38Z

production/tools/gaia_translate/src/main.cpp

@@ -2770,7 +2769,7 @@ class declarative_insert_handler_t : public MatchFinder::MatchCallback
        }
        replacement_string.resize(replacement_string.size() - 1);
        replacement_string.append("))");
-cerr<<replacement_string<<endl;
+        cerr << replacement_string << endl;


Hijacking this PR for a question: what error are we actually outputting here?

I believe it is debug output that somehow slipped into merge. It is removed in the upcoming PR with bug fixes

Well, it seems a leftover debug line :D I'll remove it.

Ok, so I'll leave it there to not conflict with greg's changes.

daxhaw · 2021-08-09T18:22:06Z

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

@@ -4638,17 +4638,22 @@ class Sema {
  bool IsExpressionInjected(const Expr* expression) const;
 private:

-    // TODO we need to decide what style to use: PascalCase, camelCase, snake_case (we're using all of them now).
+  // TODO we need to decide what style to use: PascalCase, camelCase, snake_case (we're using all of them now).


This comment can go away now, correct?

Well no, this is related to the convention used to name the actual methods within clang.

eg:

NamedDecl *injectVariableDefinition(IdentifierInfo *II, SourceLocation loc, const std::string &explicitPath); std::string ParseExplicitPath(const std::string& pathString, SourceLocation loc); .... void addField(IdentifierInfo *name, QualType type, RecordDecl *R, SourceLocation locD) const ; void RemoveExplicitPathData(SourceLocation location); StringRef ConvertString(const std::string& str, SourceLocation loc);

Apparently, clang people realized it too that they messed it up wrt naming, and they are trying to fix it in the upcoming releases... We could start by being consistent ourselves..

daxhaw · 2021-08-09T18:24:03Z

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

-if (S.getLangOpts().Gaia && S.getCurScope()->isInRulesetScope())
-{
-    DeclRefExpr *exp = dyn_cast<DeclRefExpr>(E);
+  if (S.getLangOpts().Gaia && S.getCurScope()->isInRulesetScope())


nit: why are these spaces here? clang-format goof or just typos?

We are not running .clang-format yet here, the indentation is manual (and was a little off, I fixed it here). We still need to re-activate the original LLVM .clang-format and apply it to the LLVM code...

daxhaw · 2021-08-09T18:36:50Z

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

+        {
+            // TODO we need a way to pass named params to have better diagnostics.
+            string paramName = "param_" + to_string(paramIndex);
+            ParmVarDecl* param = ParmVarDecl::Create(


I'm guessing that breaking up params into two structures is an LLVM thing? I.e., for every param you have to have a separate info structure?

That's one of those things I spent an entire week trying to understand, and honestly, I still don't. But let say that after reading and debugging clang I figured this is the way to go... :D

daxhaw · 2021-08-09T18:55:04Z

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

    }
 }


-ruleset test_connect_disconnect_invalid_syntax
+ruleset test_connect_disconnect_invalid_syntax_2


I'm probably bllind but do you have a test case that has a valid method but passes an invalid type? I.e., trying to connect/disconnect the wrong table type?

Whoopsy, ~~I'm probably bllind~~ -> "I'm definitely right" :D adding the tests.

fineg74 · 2021-08-10T21:36:09Z

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

+    // For every relationship target table we count how many links
+    // we have from tableName. This is needed to determine if we can
+    // have a connect/disconnect method for a given target type.
+    map<string, int> links_targets;


unordered_map should be slightly faster for your case

…into rondelli-conn-disconn-params-type

simone-gaia and others added 9 commits August 3, 2021 19:17

[GAIAPLAT-672] Add type safety to Connect/Disconnect params

4de12ab

- Make SemaGaia to re-use table types if already defiend.

Merge branch 'master' of https://github.com/gaia-platform/GaiaPlatform

577342a

Fix bug

265670e

Merge branch 'master' of https://github.com/gaia-platform/GaiaPlatform …

bfdb21b

…into rondelli-conn-disconn-params-type

move fbs header inc to tests

fe10491

Fix problems in gaiat

73a1bf1

Merge branch 'master' of https://github.com/gaia-platform/GaiaPlatform …

a8a7167

…into rondelli-conn-disconn-params-type

Revert "move fbs header inc to tests"

cd75876

This reverts commit fe10491.

PR fixes

e4fc06a

simone-gaia requested review from LaurentiuCristofor, phillipov, daxhaw and fineg74 August 8, 2021 19:04

LaurentiuCristofor reviewed Aug 9, 2021

View reviewed changes

Address PR feedback

0266218

daxhaw reviewed Aug 9, 2021

View reviewed changes

Add more tests

e65fe59

fineg74 reviewed Aug 10, 2021

View reviewed changes

fineg74 approved these changes Aug 10, 2021

View reviewed changes

Address PR comments

c905181

simone-gaia added 2 commits August 11, 2021 10:06

Merge branch 'master' of https://github.com/gaia-platform/GaiaPlatform …

3bcd219

…into rondelli-conn-disconn-params-type

Merge branch 'master' of https://github.com/gaia-platform/GaiaPlatform …

89e4a5b

…into rondelli-conn-disconn-params-type

simone-gaia merged commit 9d75c4d into master Aug 11, 2021

simone-gaia deleted the rondelli-conn-disconn-params-type branch August 11, 2021 15:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GAIAPLAT-672] Add type safety to connect/disconnect params #816

[GAIAPLAT-672] Add type safety to connect/disconnect params #816

simone-gaia commented Aug 8, 2021 •

edited

Loading

LaurentiuCristofor Aug 9, 2021

simone-gaia Aug 9, 2021

LaurentiuCristofor Aug 9, 2021

simone-gaia Aug 9, 2021

fineg74 Aug 10, 2021

LaurentiuCristofor Aug 9, 2021

simone-gaia Aug 9, 2021

LaurentiuCristofor Aug 9, 2021 •

edited

Loading

simone-gaia Aug 9, 2021

fineg74 Aug 10, 2021

LaurentiuCristofor Aug 9, 2021

simone-gaia Aug 9, 2021

LaurentiuCristofor Aug 9, 2021

simone-gaia Aug 9, 2021

LaurentiuCristofor left a comment

daxhaw Aug 9, 2021

fineg74 Aug 9, 2021

simone-gaia Aug 9, 2021

simone-gaia Aug 10, 2021

daxhaw Aug 9, 2021

simone-gaia Aug 9, 2021

daxhaw Aug 9, 2021

simone-gaia Aug 9, 2021 •

edited

Loading

daxhaw Aug 9, 2021

simone-gaia Aug 9, 2021

daxhaw Aug 9, 2021

simone-gaia Aug 9, 2021

fineg74 Aug 10, 2021

simone-gaia Aug 10, 2021

		@@ -497,10 +499,39 @@ void Sema::addMethod(IdentifierInfo* name, DeclSpec::TST retValType, DeclaratorC

		DS.Finish(*this, getPrintingPolicy());

		SmallVector<DeclaratorChunk::ParamInfo, 8> paramsInfo;

		@@ -1,27 +1,27 @@
		CREATE DATABASE if not exists incubator;
		create database if not exists incubator;

[GAIAPLAT-672] Add type safety to connect/disconnect params #816

[GAIAPLAT-672] Add type safety to connect/disconnect params #816

Conversation

simone-gaia commented Aug 8, 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

Choose a reason for hiding this comment

LaurentiuCristofor Aug 9, 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

Choose a reason for hiding this comment

LaurentiuCristofor left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simone-gaia Aug 9, 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

Choose a reason for hiding this comment

simone-gaia commented Aug 8, 2021 •

edited

Loading

LaurentiuCristofor Aug 9, 2021 •

edited

Loading

simone-gaia Aug 9, 2021 •

edited

Loading