Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 14 commits into from
Aug 11, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
4de12ab
[GAIAPLAT-672] Add type safety to Connect/Disconnect params
simone-gaia Jul 29, 2021
577342a
Merge branch 'master' of https://github.com/gaia-platform/GaiaPlatform
simone-gaia Aug 3, 2021
265670e
Fix bug
simone-gaia Aug 3, 2021
bfdb21b
Merge branch 'master' of https://github.com/gaia-platform/GaiaPlatfor…
simone-gaia Aug 3, 2021
fe10491
move fbs header inc to tests
chuan Aug 3, 2021
73a1bf1
Fix problems in gaiat
simone-gaia Aug 6, 2021
a8a7167
Merge branch 'master' of https://github.com/gaia-platform/GaiaPlatfor…
simone-gaia Aug 8, 2021
cd75876
Revert "move fbs header inc to tests"
simone-gaia Aug 8, 2021
e4fc06a
PR fixes
simone-gaia Aug 8, 2021
0266218
Address PR feedback
simone-gaia Aug 9, 2021
e65fe59
Add more tests
simone-gaia Aug 9, 2021
c905181
Address PR comments
simone-gaia Aug 10, 2021
3bcd219
Merge branch 'master' of https://github.com/gaia-platform/GaiaPlatfor…
simone-gaia Aug 11, 2021
89e4a5b
Merge branch 'master' of https://github.com/gaia-platform/GaiaPlatfor…
simone-gaia Aug 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 10 additions & 7 deletions production/tools/gaia_translate/src/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
#pragma clang diagnostic pop

#include "gaia_internal/common/gaia_version.hpp"
#include "gaia_internal/common/system_error.hpp"
#include "gaia_internal/db/db_client_config.hpp"
#include "gaia_internal/db/gaia_db_internal.hpp"

Expand Down Expand Up @@ -2828,7 +2827,7 @@ class declarative_insert_handler_t : public MatchFinder::MatchCallback
SourceRange(argument_start_location, argument->getSourceRange().getEnd()));
size_t argument_name_end_position = raw_argument_name.find(':');
string argument_name = raw_argument_name.substr(0, argument_name_end_position);
//Trim the argument name of whitespaces.
// Trim the argument name of whitespaces.
argument_name.erase(argument_name.begin(), find_if(argument_name.begin(), argument_name.end(), [](unsigned char ch) { return !isspace(ch); }));
argument_name.erase(find_if(argument_name.rbegin(), argument_name.rend(), [](unsigned char ch) { return !isspace(ch); }).base(), argument_name.end());
insert_data.argument_map[argument_name] = argument->getSourceRange();
Expand Down Expand Up @@ -3126,10 +3125,12 @@ class translation_engine_consumer_t : public clang::ASTConsumer
hasAttr(attr::GaiaField),
unless(hasAttr(attr::GaiaFieldLValue)))),
hasDescendant(declRefExpr(
to(varDecl(anyOf(
hasAttr(attr::GaiaField),
hasAttr(attr::FieldTable),
hasAttr(attr::GaiaFieldValue)))))))
to(varDecl(
anyOf(
hasAttr(attr::GaiaField),
hasAttr(attr::FieldTable),
hasAttr(attr::GaiaFieldValue)),
unless(hasAttr(attr::GaiaFieldLValue)))))))
.bind("tableFieldGet");
StatementMatcher table_field_set_matcher
= binaryOperator(
Expand All @@ -3138,7 +3139,9 @@ class translation_engine_consumer_t : public clang::ASTConsumer
isAssignmentOperator(),
hasLHS(
memberExpr(
member(hasAttr(attr::GaiaFieldLValue))))))
hasDescendant(
declRefExpr(
to(varDecl(hasAttr(attr::GaiaFieldLValue)))))))))
.bind("fieldSet");
StatementMatcher table_field_unary_operator_matcher
= unaryOperator(allOf(
Expand Down
12 changes: 6 additions & 6 deletions ...d_party/production/TranslationEngineLLVM/clang/include/clang/Basic/DiagnosticSemaKinds.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9505,21 +9505,21 @@ def err_invalid_field_type : Error<
def err_no_ruleset_for_rule : Error<
"No ruleset was detected for rule.">;
def err_ambiguous_field_reference : Error<
"Ambiguous reference to field '%0'.">;
"Ambiguous reference to field '%0'.">;
def note_table_reference : Note<
"Table: '%0'.">;
"Table: '%0'.">;
def note_field_reference : Note<
"Field: '%0.%1'.">;
"Field: '%0.%1'.">;
def note_link_reference : Note<
"Link: '%0.%1'.">;
"Link: '%0.%1'.">;
def err_duplicate_field : Error<
"Duplicate field '%0'.">;
def err_duplicate_link : Error<
"Duplicate link '%0'.">;
"Duplicate link '%0'.">;
def err_unknown_field : Error<
"Field '%0' was not found in the catalog.">;
def err_catalog_exception : Error<
"Exception occured while fetching data from catalog: '%0'.">;
"Exception occurred while fetching data from catalog: '%0'.">;
def err_invalid_table_field : Error<
"Field '%0' has no table in the catalog.">;
def warn_table_referenced_not_in_table_attribute : Warning<
Expand Down
15 changes: 10 additions & 5 deletions third_party/production/TranslationEngineLLVM/clang/include/clang/Sema/Sema.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment can go away now, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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..

NamedDecl *injectVariableDefinition(IdentifierInfo *II, SourceLocation loc, const std::string &explicitPath);
std::string ParseExplicitPath(const std::string& pathString, SourceLocation loc);
QualType getFieldType (const std::string& fieldOrTagName, SourceLocation loc);
QualType getTableType (const std::string &tableName, SourceLocation loc);
std::unordered_map<std::string, std::string> getTagMapping(const DeclContext *context, SourceLocation loc);
QualType getRuleContextType(SourceLocation loc);
QualType getLinkType(const std::string& linkName, const std::string& from_table, SourceLocation loc);
void addMethod(IdentifierInfo *name, DeclSpec::TST retValType, DeclaratorChunk::ParamInfo *Params,
unsigned NumParams, AttributeFactory &attrFactory, ParsedAttributes &attrs, Scope *S, RecordDecl *RD,
SourceLocation loc, bool isVariadic = false, ParsedType returnType = nullptr) ;
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);

/// Lookup a class name in the given context. Returns nullptr if the class is not found.
/// If the class has been defined in this context (eg. "class x {};") the defined type is
/// returned otherwise only the forward declaration is returned (eg. "class x;").
TagDecl* lookupClass(std::string className, SourceLocation loc, Scope* scope);
void addField(IdentifierInfo *name, QualType type, RecordDecl *R, SourceLocation locD) const ;
void RemoveExplicitPathData(SourceLocation location);
StringRef ConvertString(const std::string& str, SourceLocation loc);
Expand Down
52 changes: 24 additions & 28 deletions third_party/production/TranslationEngineLLVM/clang/lib/Sema/SemaExpr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11323,36 +11323,32 @@ static void DiagnoseRecursiveConstFields(Sema &S, const Expr *E,
static bool CheckForModifiableLvalue(Expr *E, SourceLocation Loc, Sema &S) {
assert(!E->hasPlaceholderType(BuiltinType::PseudoObject));

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@simone-gaia simone-gaia Aug 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

{
DeclRefExpr* exp = dyn_cast<DeclRefExpr>(E);

if (exp != nullptr)
{
ValueDecl *decl = exp->getDecl();
if (exp == nullptr)
{
MemberExpr* memberExp = dyn_cast<MemberExpr>(E);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...


if (decl->hasAttr<GaiaFieldAttr>() ||
decl->hasAttr<GaiaFieldValueAttr>() ||
decl->hasAttr<FieldTableAttr>())
{
decl->addAttr(GaiaFieldLValueAttr::CreateImplicit(S.Context));
}
}
else
{
MemberExpr *memberExp = dyn_cast<MemberExpr>(E);
if (memberExp != nullptr)
{
ValueDecl *decl = memberExp->getMemberDecl();
if (decl->hasAttr<GaiaFieldAttr>() ||
decl->hasAttr<GaiaFieldValueAttr>() ||
decl->hasAttr<FieldTableAttr>())
{
decl->addAttr(GaiaFieldLValueAttr::CreateImplicit(S.Context));
}
}
}
}
if (memberExp != nullptr)
{
exp = dyn_cast<DeclRefExpr>(memberExp->getBase());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

if (exp != nullptr)
{
ValueDecl* decl = exp->getDecl();

if (decl->hasAttr<GaiaFieldAttr>()
|| decl->hasAttr<GaiaFieldValueAttr>()
|| decl->hasAttr<FieldTableAttr>())
{
decl->addAttr(GaiaFieldLValueAttr::CreateImplicit(S.Context));
}
}
}

S.CheckShadowingDeclModification(E, Loc);

Expand Down
Loading