Skip to content

Fix the 'dynamic_cast' not permitted with '-fno-rtti' error when integrated with LLVM/MLIR.#217

Merged
Bouncner merged 4 commits intohyrise:masterfrom
mengke-mk:dynamic_cast-patch-1
Aug 30, 2022
Merged

Fix the 'dynamic_cast' not permitted with '-fno-rtti' error when integrated with LLVM/MLIR.#217
Bouncner merged 4 commits intohyrise:masterfrom
mengke-mk:dynamic_cast-patch-1

Conversation

@mengke-mk
Copy link
Copy Markdown
Contributor

A compile error occurs when including the "SQLParser.h" in another project. (sql_parser as third-party)

/thirdparty/sql-parser/src/sql/CreateStatement.h:74:67: error: 'dynamic_cast' not permitted with '-fno-rtti'
   74 | ynamic_cast<ColumnDefinition*>(tableElem)) {
      |                                         ^

/thirdparty/sql-parser/src/sql/CreateStatement.h:76:82: error: 'dynamic_cast' not permitted with '-fno-rtti'
   76 | dynamic_cast<TableConstraint*>(tableElem)) {
      |                                         ^

Separate the declaration and definition of the setColumnDefsAndConstraints should fix this.

@Bouncner
Copy link
Copy Markdown
Collaborator

Bouncner commented Aug 3, 2022

Any chance to can provide an example of how to trigger this issue?

Which clang version do you use?

@mengke-mk
Copy link
Copy Markdown
Contributor Author

The definition of setColumnDefsAndConstraints is in header file, and it tires to call dynamic_cast.
LLVM/MLIR project implicitly add -fno-rtti flag (this flag disable dynamic_cast) and this will cause a compilation error as shown in the title.

Add -fno-rtti in example/Makefile will be an MRE.

#example/Makefile
CFLAGS = -std=c++1z -lstdc++ -Wall -fno-rtti -Werror -I../src/ -L../
all:
     $(CXX) $(CFLAGS) example.cpp -o example -lsqlparser

@mengke-mk
Copy link
Copy Markdown
Contributor Author

Hi @Bouncner, any chance to get this landed? This fix does not introduce any changes but only removes the function definition from the header file.

Copy link
Copy Markdown
Collaborator

@Bouncner Bouncner left a comment

Choose a reason for hiding this comment

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

We just had our weekly discussion today. We certainly welcome your PR.

One small thing.
In the long run, we would like to remove the cluttered statements.cpp. For that, could you please move the function to a new CreateStatement.cpp?

@mengke-mk
Copy link
Copy Markdown
Contributor Author

In the long run, we would like to remove the cluttered statements.cpp. For that, could you please move the function to a new CreateStatement.cpp?

Hi @Bouncner, the above issue has been addressed, PTAL.

@mengke-mk mengke-mk requested a review from Bouncner August 11, 2022 03:26
Comment thread src/sql/CreateStatement.cpp
@mengke-mk mengke-mk force-pushed the dynamic_cast-patch-1 branch from c7075a3 to aaee1ab Compare August 11, 2022 12:45
@Bouncner
Copy link
Copy Markdown
Collaborator

Feel free to merge.

@Bouncner Bouncner merged commit 44f66fd into hyrise:master Aug 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants