Add llvm support for meta rule 3 #1118
Conversation
llvm library for potential use on llvm side.
…e and add the attribute to pass it to Translation Engine
| @@ -9529,6 +9529,8 @@ def err_duplicate_field : Error< | |||
| "table name (table.field) to disambiguate field names that occur in more " | |||
| "than one table. You can also restrict the list of tables to search by " | |||
| "specifying them in the ruleset 'tables' attribute.">; | |||
| def err_wrong_table_field : Error< | |||
| "Field '%0' belongs to a table '%1' that does not exist in the catalog.">; | |||
There was a problem hiding this comment.
I think it sounds strange to say that a field belongs to a table that does not exist, because by saying "belong" it seems the table somewhat exists.
Probably @vDonGlover knows a better way to say this or, what I said above, doesn't make sense.
There was a problem hiding this comment.
Maybe instead of "belongs" we can say "is marked as belonging".
There was a problem hiding this comment.
"is defined in a table that does not exist"?
| return return_value; | ||
| } | ||
|
|
||
| // Find shortest navigation path between 2 tables. If multiple shortest paths exist, return an error. |
There was a problem hiding this comment.
What does it mean "return an error" since this function returns true/false?
| } | ||
|
|
||
| // Find shortest navigation path between 2 tables. If multiple shortest paths exist, return an error. | ||
| bool GaiaCatalog::findNavigationPath(StringRef src, StringRef dst, SmallVector<string, 8>& current_path, bool reportErrors) |
There was a problem hiding this comment.
What is current_path? And what is reportErrors? Can you add documentation for the parameters?
| } | ||
| const auto& table_data = getCatalogTableData(); | ||
|
|
||
| IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts = new DiagnosticOptions(); |
There was a problem hiding this comment.
Since you are inside "gaia code" use snake_case (or use CamelCase for Gaia stuff).
| TextDiagnosticPrinter *DiagClient = new TextDiagnosticPrinter(llvm::errs(), &*DiagOpts); | ||
|
|
||
| IntrusiveRefCntPtr<DiagnosticIDs> DiagID(new DiagnosticIDs()); | ||
| DiagnosticsEngine Diags(DiagID, &*DiagOpts, DiagClient); |
There was a problem hiding this comment.
Does this allocated memory need to be released?
There was a problem hiding this comment.
IntrusiveRefCntPtr is a smart pointer.
Diagnostic Engine uses smart pointers for Diagnostic Client inside. So memory is taken care of
There was a problem hiding this comment.
I'd add a comment for DiagClient, to indicate this.
| if (foundTables.size() == 1) | ||
| { | ||
| fieldTableName = foundTables.begin()->first(); | ||
| retVal = foundTables.begin()->second; |
There was a problem hiding this comment.
Instead of retVal, you could just use result. Same number of characters, no abbreviation.
| // Find topographically shortest path between anchor table and destination table. | ||
| if (gaia::catalog::GaiaCatalog::findNavigationPath(anchor_table_iterator->first(), firstComponent, path, false)) | ||
| { | ||
| if(path.size() < pathLength) |
There was a problem hiding this comment.
Since @waynelwarren is busy with other matters somebody else's need to write extensive tests for this feature. Are you planning to do it as part of this PR ?
There are tests covering what was introduced in this PR (Resolving an unqualified field reference in some cases that earlier was considered ambiguous ). The more interesting part would be code generation where more tests would come in separate PR. |
Changed |
The PR changes a way how type of an unqualified field is determined and attributes every declarative reference with potential anchor table reference that may be used by Translation Engine to generate the navigation code properly